Re: [libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-02-19 Thread Luca Barbato 
On 19/02/2017 23:02, John Stebbins wrote:
> On 02/19/2017 10:25 AM, Luca Barbato wrote:
>> On 19/02/2017 17:15, John Stebbins wrote:
>>> ---
>>>  libavformat/dv.c | 7 +--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavformat/dv.c b/libavformat/dv.c
>>> index d4e5180..0cd60de 100644
>>> --- a/libavformat/dv.c
>>> +++ b/libavformat/dv.c
>>> @@ -484,11 +484,14 @@ static int dv_read_packet(AVFormatContext *s, 
>>> AVPacket *pkt)
>>>  size = avpriv_dv_get_packet(c->dv_demux, pkt);
>>>  
>>>  if (size < 0) {
>>> +int result;
>>> +
>>>  if (!c->dv_demux->sys)
>>>  return AVERROR(EIO);
>>>  size = c->dv_demux->sys->frame_size;
>>> -if (avio_read(s->pb, c->buf, size) <= 0)
>>> -return AVERROR(EIO);
>>> +result = avio_read(s->pb, c->buf, size);
>>> +if (result <= 0)
>>> +return result;
>>>  
>>>  size = avpriv_dv_produce_packet(c->dv_demux, pkt, c->buf, size);
>>>  }
>>>
>> Probably it is a stable candidate.
>>
>> nit: `ret` is the name normally used across the codebase for the
>> variable instead of result.
>>
>>
> 
> Yes, it would be nice for this to go into stable. result changed to ret 
> locally.
> 
> May I push?
> 

Sure :)

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-02-19 Thread John Stebbins 
On 02/19/2017 10:25 AM, Luca Barbato wrote:
> On 19/02/2017 17:15, John Stebbins wrote:
>> ---
>>  libavformat/dv.c | 7 +--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/dv.c b/libavformat/dv.c
>> index d4e5180..0cd60de 100644
>> --- a/libavformat/dv.c
>> +++ b/libavformat/dv.c
>> @@ -484,11 +484,14 @@ static int dv_read_packet(AVFormatContext *s, AVPacket 
>> *pkt)
>>  size = avpriv_dv_get_packet(c->dv_demux, pkt);
>>  
>>  if (size < 0) {
>> +int result;
>> +
>>  if (!c->dv_demux->sys)
>>  return AVERROR(EIO);
>>  size = c->dv_demux->sys->frame_size;
>> -if (avio_read(s->pb, c->buf, size) <= 0)
>> -return AVERROR(EIO);
>> +result = avio_read(s->pb, c->buf, size);
>> +if (result <= 0)
>> +return result;
>>  
>>  size = avpriv_dv_produce_packet(c->dv_demux, pkt, c->buf, size);
>>  }
>>
> Probably it is a stable candidate.
>
> nit: `ret` is the name normally used across the codebase for the
> variable instead of result.
>
>

Yes, it would be nice for this to go into stable. result changed to ret locally.

May I push?

-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-02-19 Thread Luca Barbato 
On 19/02/2017 17:15, John Stebbins wrote:
> ---
>  libavformat/dv.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/dv.c b/libavformat/dv.c
> index d4e5180..0cd60de 100644
> --- a/libavformat/dv.c
> +++ b/libavformat/dv.c
> @@ -484,11 +484,14 @@ static int dv_read_packet(AVFormatContext *s, AVPacket 
> *pkt)
>  size = avpriv_dv_get_packet(c->dv_demux, pkt);
>  
>  if (size < 0) {
> +int result;
> +
>  if (!c->dv_demux->sys)
>  return AVERROR(EIO);
>  size = c->dv_demux->sys->frame_size;
> -if (avio_read(s->pb, c->buf, size) <= 0)
> -return AVERROR(EIO);
> +result = avio_read(s->pb, c->buf, size);
> +if (result <= 0)
> +return result;
>  
>  size = avpriv_dv_produce_packet(c->dv_demux, pkt, c->buf, size);
>  }
> 

Probably it is a stable candidate.

nit: `ret` is the name normally used across the codebase for the
variable instead of result.

lu
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-02-19 Thread John Stebbins 
---
 libavformat/dv.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/libavformat/dv.c b/libavformat/dv.c
index d4e5180..0cd60de 100644
--- a/libavformat/dv.c
+++ b/libavformat/dv.c
@@ -484,11 +484,14 @@ static int dv_read_packet(AVFormatContext *s, AVPacket 
*pkt)
 size = avpriv_dv_get_packet(c->dv_demux, pkt);
 
 if (size < 0) {
+int result;
+
 if (!c->dv_demux->sys)
 return AVERROR(EIO);
 size = c->dv_demux->sys->frame_size;
-if (avio_read(s->pb, c->buf, size) <= 0)
-return AVERROR(EIO);
+result = avio_read(s->pb, c->buf, size);
+if (result <= 0)
+return result;
 
 size = avpriv_dv_produce_packet(c->dv_demux, pkt, c->buf, size);
 }
-- 
2.9.3

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-02-19 Thread Luca Barbato 
On 18/02/2017 22:40, John Stebbins wrote:
> On 02/18/2017 01:20 PM, Luca Barbato wrote:
>> On 18/02/2017 14:37, John Stebbins wrote:
>>
>>> I forgot this was still pending.  I never got an answer to my last
>>> question above.  Do you all think wm4's suggestion is sensible. His
>>> suggestion results in dv_read_packet returning 0 instead of EIO when
>>> avio_read returns 0. IMO, if this change results in other issues,
>>> those issues should be fixed since they rely on broken behaviour, so
>>> my opinion is wm4's suggestion is good.
>>>
>> There is a pb->eof_reached for it.
>>
>>
> 
> I'm not sure what you are getting at.  avio_read will return AVERROR_EOF when 
> EOF is reached and AVIOContext buffer has
> been consumed. So there is no need to look at pb->eof_reached in dv.c. In 
> fact, I think it's not possible for avio_read
> to return 0.  It looks like all scenarios that would result in 0 either 
> generate an error or eof.
> 

Then I guess we are set :)

lu

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-02-18 Thread Luca Barbato 
On 18/02/2017 14:37, John Stebbins wrote:

> I forgot this was still pending.  I never got an answer to my last
> question above.  Do you all think wm4's suggestion is sensible. His
> suggestion results in dv_read_packet returning 0 instead of EIO when
> avio_read returns 0. IMO, if this change results in other issues,
> those issues should be fixed since they rely on broken behaviour, so
> my opinion is wm4's suggestion is good.
> 

There is a pb->eof_reached for it.

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-02-18 Thread John Stebbins 
On 01/14/2017 12:46 PM, John Stebbins wrote:
> On 01/14/2017 12:30 PM, John Stebbins wrote:
>> On 01/12/2017 10:51 PM, wm4 wrote:
>>> On Thu, 12 Jan 2017 10:33:28 -0700
>>> John Stebbins  wrote:
>>>
 ---
  libavformat/dv.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

 diff --git a/libavformat/dv.c b/libavformat/dv.c
 index d4e5180..3ff369b 100644
 --- a/libavformat/dv.c
 +++ b/libavformat/dv.c
 @@ -484,10 +484,15 @@ static int dv_read_packet(AVFormatContext *s, 
 AVPacket *pkt)
  size = avpriv_dv_get_packet(c->dv_demux, pkt);
  
  if (size < 0) {
 +int result;
 +
  if (!c->dv_demux->sys)
  return AVERROR(EIO);
  size = c->dv_demux->sys->frame_size;
 -if (avio_read(s->pb, c->buf, size) <= 0)
 +result = avio_read(s->pb, c->buf, size);
 +if (result == AVERROR_EOF)
 +return result;
 +if (result <= 0)
  return AVERROR(EIO);
  
  size = avpriv_dv_produce_packet(c->dv_demux, pkt, c->buf, size);
>>> Why not just return the error code as-is?
>>>
>>> While I have my doubts whether it's useful, it's simpler and is what
>>> most other demuxers (probably) do.
>>>
>> Ok by me.  I was just making the minimal change to behaviour possible in 
>> case the current behaviour mattered to somebody.
>>
> After a closer look, is avio_read guaranteed to return AVERROR_EOF upon end 
> of input for all types of AVIOContext?  I
> see some code that explicitly checks for avio_read returning 0 and treating 
> that as EOF. I.e. how should I handle a
> return value of 0 from avio_read?
>

I forgot this was still pending.  I never got an answer to my last question 
above.  Do you all think wm4's suggestion is
sensible. His suggestion results in dv_read_packet returning 0 instead of EIO 
when avio_read returns 0. IMO, if this
change results in other issues, those issues should be fixed since they rely on 
broken behaviour, so my opinion is wm4's
suggestion is good.

-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-01-14 Thread John Stebbins 
On 01/14/2017 12:30 PM, John Stebbins wrote:
> On 01/12/2017 10:51 PM, wm4 wrote:
>> On Thu, 12 Jan 2017 10:33:28 -0700
>> John Stebbins  wrote:
>>
>>> ---
>>>  libavformat/dv.c | 7 ++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/dv.c b/libavformat/dv.c
>>> index d4e5180..3ff369b 100644
>>> --- a/libavformat/dv.c
>>> +++ b/libavformat/dv.c
>>> @@ -484,10 +484,15 @@ static int dv_read_packet(AVFormatContext *s, 
>>> AVPacket *pkt)
>>>  size = avpriv_dv_get_packet(c->dv_demux, pkt);
>>>  
>>>  if (size < 0) {
>>> +int result;
>>> +
>>>  if (!c->dv_demux->sys)
>>>  return AVERROR(EIO);
>>>  size = c->dv_demux->sys->frame_size;
>>> -if (avio_read(s->pb, c->buf, size) <= 0)
>>> +result = avio_read(s->pb, c->buf, size);
>>> +if (result == AVERROR_EOF)
>>> +return result;
>>> +if (result <= 0)
>>>  return AVERROR(EIO);
>>>  
>>>  size = avpriv_dv_produce_packet(c->dv_demux, pkt, c->buf, size);
>> Why not just return the error code as-is?
>>
>> While I have my doubts whether it's useful, it's simpler and is what
>> most other demuxers (probably) do.
>>
> Ok by me.  I was just making the minimal change to behaviour possible in case 
> the current behaviour mattered to somebody.
>

After a closer look, is avio_read guaranteed to return AVERROR_EOF upon end of 
input for all types of AVIOContext?  I
see some code that explicitly checks for avio_read returning 0 and treating 
that as EOF. I.e. how should I handle a
return value of 0 from avio_read?

-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-01-14 Thread John Stebbins 
On 01/12/2017 10:51 PM, wm4 wrote:
> On Thu, 12 Jan 2017 10:33:28 -0700
> John Stebbins  wrote:
>
>> ---
>>  libavformat/dv.c | 7 ++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/dv.c b/libavformat/dv.c
>> index d4e5180..3ff369b 100644
>> --- a/libavformat/dv.c
>> +++ b/libavformat/dv.c
>> @@ -484,10 +484,15 @@ static int dv_read_packet(AVFormatContext *s, AVPacket 
>> *pkt)
>>  size = avpriv_dv_get_packet(c->dv_demux, pkt);
>>  
>>  if (size < 0) {
>> +int result;
>> +
>>  if (!c->dv_demux->sys)
>>  return AVERROR(EIO);
>>  size = c->dv_demux->sys->frame_size;
>> -if (avio_read(s->pb, c->buf, size) <= 0)
>> +result = avio_read(s->pb, c->buf, size);
>> +if (result == AVERROR_EOF)
>> +return result;
>> +if (result <= 0)
>>  return AVERROR(EIO);
>>  
>>  size = avpriv_dv_produce_packet(c->dv_demux, pkt, c->buf, size);
> Why not just return the error code as-is?
>
> While I have my doubts whether it's useful, it's simpler and is what
> most other demuxers (probably) do.
>

Ok by me.  I was just making the minimal change to behaviour possible in case 
the current behaviour mattered to somebody.

-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-01-12 Thread wm4 
On Thu, 12 Jan 2017 10:33:28 -0700
John Stebbins  wrote:

> ---
>  libavformat/dv.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/dv.c b/libavformat/dv.c
> index d4e5180..3ff369b 100644
> --- a/libavformat/dv.c
> +++ b/libavformat/dv.c
> @@ -484,10 +484,15 @@ static int dv_read_packet(AVFormatContext *s, AVPacket 
> *pkt)
>  size = avpriv_dv_get_packet(c->dv_demux, pkt);
>  
>  if (size < 0) {
> +int result;
> +
>  if (!c->dv_demux->sys)
>  return AVERROR(EIO);
>  size = c->dv_demux->sys->frame_size;
> -if (avio_read(s->pb, c->buf, size) <= 0)
> +result = avio_read(s->pb, c->buf, size);
> +if (result == AVERROR_EOF)
> +return result;
> +if (result <= 0)
>  return AVERROR(EIO);
>  
>  size = avpriv_dv_produce_packet(c->dv_demux, pkt, c->buf, size);

Why not just return the error code as-is?

While I have my doubts whether it's useful, it's simpler and is what
most other demuxers (probably) do.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-01-12 Thread Diego Biurrun 
On Thu, Jan 12, 2017 at 10:33:28AM -0700, John Stebbins wrote:
> ---
>  libavformat/dv.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

OK

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-01-12 Thread Diego Biurrun 
On Thu, Jan 12, 2017 at 10:27:59AM -0700, John Stebbins wrote:
> On 01/12/2017 05:31 AM, Diego Biurrun wrote:
> > On Wed, Jan 11, 2017 at 12:22:10PM -0700, John Stebbins wrote:
> >> --- a/libavformat/dv.c
> >> +++ b/libavformat/dv.c
> >> @@ -478,7 +478,7 @@ static int dv_read_header(AVFormatContext *s)
> >>  static int dv_read_packet(AVFormatContext *s, AVPacket *pkt)
> >>  {
> >> -int size;
> >> +int size, result;
> >>  RawDVContext *c = s->priv_data;
> >>  
> >> @@ -487,7 +487,10 @@ static int dv_read_packet(AVFormatContext *s, 
> >> AVPacket *pkt)
> >>  if (!c->dv_demux->sys)
> >>  return AVERROR(EIO);
> >>  size = c->dv_demux->sys->frame_size;
> >> -if (avio_read(s->pb, c->buf, size) <= 0)
> >> +result = avio_read(s->pb, c->buf, size);
> >> +if (result == AVERROR_EOF)
> >> +return result;
> >> +if (result <= 0)
> >>  return AVERROR(EIO);
> > nit: The variable could have smaller scope.
> Ok. Will submit update to fix.

Feel free to fix that on push in the future.

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-01-12 Thread John Stebbins 
---
 libavformat/dv.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/libavformat/dv.c b/libavformat/dv.c
index d4e5180..3ff369b 100644
--- a/libavformat/dv.c
+++ b/libavformat/dv.c
@@ -484,10 +484,15 @@ static int dv_read_packet(AVFormatContext *s, AVPacket 
*pkt)
 size = avpriv_dv_get_packet(c->dv_demux, pkt);
 
 if (size < 0) {
+int result;
+
 if (!c->dv_demux->sys)
 return AVERROR(EIO);
 size = c->dv_demux->sys->frame_size;
-if (avio_read(s->pb, c->buf, size) <= 0)
+result = avio_read(s->pb, c->buf, size);
+if (result == AVERROR_EOF)
+return result;
+if (result <= 0)
 return AVERROR(EIO);
 
 size = avpriv_dv_produce_packet(c->dv_demux, pkt, c->buf, size);
-- 
2.9.3

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-01-12 Thread John Stebbins 
On 01/12/2017 05:31 AM, Diego Biurrun wrote:
> On Wed, Jan 11, 2017 at 12:22:10PM -0700, John Stebbins wrote:
>> --- a/libavformat/dv.c
>> +++ b/libavformat/dv.c
>> @@ -478,7 +478,7 @@ static int dv_read_header(AVFormatContext *s)
>>  static int dv_read_packet(AVFormatContext *s, AVPacket *pkt)
>>  {
>> -int size;
>> +int size, result;
>>  RawDVContext *c = s->priv_data;
>>  
>> @@ -487,7 +487,10 @@ static int dv_read_packet(AVFormatContext *s, AVPacket 
>> *pkt)
>>  if (!c->dv_demux->sys)
>>  return AVERROR(EIO);
>>  size = c->dv_demux->sys->frame_size;
>> -if (avio_read(s->pb, c->buf, size) <= 0)
>> +result = avio_read(s->pb, c->buf, size);
>> +if (result == AVERROR_EOF)
>> +return result;
>> +if (result <= 0)
>>  return AVERROR(EIO);
> nit: The variable could have smaller scope.
Ok. Will submit update to fix.
>
> Is there a specific problem that this fixes?
Yes.  HandBrake shows that encodes do not finish successfully because we 
receive EIO from libav instead of EOF.


-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-01-12 Thread Diego Biurrun 
On Wed, Jan 11, 2017 at 12:22:10PM -0700, John Stebbins wrote:
> --- a/libavformat/dv.c
> +++ b/libavformat/dv.c
> @@ -478,7 +478,7 @@ static int dv_read_header(AVFormatContext *s)
>  static int dv_read_packet(AVFormatContext *s, AVPacket *pkt)
>  {
> -int size;
> +int size, result;
>  RawDVContext *c = s->priv_data;
>  
> @@ -487,7 +487,10 @@ static int dv_read_packet(AVFormatContext *s, AVPacket 
> *pkt)
>  if (!c->dv_demux->sys)
>  return AVERROR(EIO);
>  size = c->dv_demux->sys->frame_size;
> -if (avio_read(s->pb, c->buf, size) <= 0)
> +result = avio_read(s->pb, c->buf, size);
> +if (result == AVERROR_EOF)
> +return result;
> +if (result <= 0)
>  return AVERROR(EIO);

nit: The variable could have smaller scope.

Is there a specific problem that this fixes?

probably OK

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [PATCH] dv: Don't return EIO upon EOF

2017-01-11 Thread John Stebbins 
---
 libavformat/dv.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/libavformat/dv.c b/libavformat/dv.c
index d4e5180..7e52e42 100644
--- a/libavformat/dv.c
+++ b/libavformat/dv.c
@@ -478,7 +478,7 @@ static int dv_read_header(AVFormatContext *s)
 
 static int dv_read_packet(AVFormatContext *s, AVPacket *pkt)
 {
-int size;
+int size, result;
 RawDVContext *c = s->priv_data;
 
 size = avpriv_dv_get_packet(c->dv_demux, pkt);
@@ -487,7 +487,10 @@ static int dv_read_packet(AVFormatContext *s, AVPacket 
*pkt)
 if (!c->dv_demux->sys)
 return AVERROR(EIO);
 size = c->dv_demux->sys->frame_size;
-if (avio_read(s->pb, c->buf, size) <= 0)
+result = avio_read(s->pb, c->buf, size);
+if (result == AVERROR_EOF)
+return result;
+if (result <= 0)
 return AVERROR(EIO);
 
 size = avpriv_dv_produce_packet(c->dv_demux, pkt, c->buf, size);
-- 
2.9.3

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel