dv: Don't return EIO upon EOF

Message ID 20170112173328.7661-1-stebbins@jetheaddev.com
State New
Headers show

Commit Message

John Stebbins Jan. 12, 2017, 5:33 p.m.
---
 libavformat/dv.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Diego Biurrun Jan. 12, 2017, 7 p.m. | #1
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
wm4 Jan. 13, 2017, 5:51 a.m. | #2
On Thu, 12 Jan 2017 10:33:28 -0700
John Stebbins <stebbins@jetheaddev.com> 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.
John Stebbins Jan. 14, 2017, 7:30 p.m. | #3
On 01/12/2017 10:51 PM, wm4 wrote:
> On Thu, 12 Jan 2017 10:33:28 -0700
> John Stebbins <stebbins@jetheaddev.com> 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 Stebbins Jan. 14, 2017, 7:46 p.m. | #4
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 <stebbins@jetheaddev.com> 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 Stebbins Feb. 18, 2017, 1:37 p.m. | #5
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 <stebbins@jetheaddev.com> 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.
Luca Barbato Feb. 18, 2017, 8:20 p.m. | #6
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.
John Stebbins Feb. 18, 2017, 9:40 p.m. | #7
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.
Luca Barbato Feb. 19, 2017, 8:52 a.m. | #8
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

Patch

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);