[FFmpeg-devel] avformat/utils: return pending IO error on EOF in av_read_frame()

Submitted by Marton Balint on Aug. 24, 2019, 8:40 p.m.

Details

Message ID 20190824204051.21226-1-cus@passwd.hu
State Accepted
Commit ce1fcc8cede2971f6e36119e9dd41fb41b36c63a
Headers show

Commit Message

Marton Balint Aug. 24, 2019, 8:40 p.m.
avio_feof() returns true both in case of actual EOF and in case of IO errors.
Some demuxers (matroska) have special handling to be able to return the proper
error for this exact reason, e.g.:

if (avio_feof(pb)) {
     if (pb->error) {
         return pb->error;
     } else {
         return AVERROR_EOF;
     }
}

However, most of the demuxers do not, and they simply return AVERROR_EOF if
avio_feof() is true, so there is a real chance that IO errors are mistaken for
EOF.

We might just say that the API user should always check the IO context error
attribute on EOF to make sure no IO errors happened, but not even ffmpeg.c does
this. It should be more intuitive to the API user if we simply return the IO
error as the return value of av_read_frame() instead of AVERROR_EOF.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/utils.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Marton Balint Aug. 29, 2019, 7:57 p.m.
On Sat, 24 Aug 2019, Marton Balint wrote:

> avio_feof() returns true both in case of actual EOF and in case of IO errors.
> Some demuxers (matroska) have special handling to be able to return the proper
> error for this exact reason, e.g.:
>
> if (avio_feof(pb)) {
>     if (pb->error) {
>         return pb->error;
>     } else {
>         return AVERROR_EOF;
>     }
> }
>
> However, most of the demuxers do not, and they simply return AVERROR_EOF if
> avio_feof() is true, so there is a real chance that IO errors are mistaken for
> EOF.
>
> We might just say that the API user should always check the IO context error
> attribute on EOF to make sure no IO errors happened, but not even ffmpeg.c does
> this. It should be more intuitive to the API user if we simply return the IO
> error as the return value of av_read_frame() instead of AVERROR_EOF.
>
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
> libavformat/utils.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index b57e680089..b83a740500 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -1762,6 +1762,11 @@ FF_ENABLE_DEPRECATION_WARNINGS
>                av_ts2str(pkt->dts),
>                pkt->size, pkt->duration, pkt->flags);
> 
> +    /* A demuxer might have returned EOF because of an IO error, let's
> +     * propagate this back to the user. */
> +    if (ret == AVERROR_EOF && s->pb && s->pb->error < 0 && s->pb->error != AVERROR(EAGAIN))
> +        ret = s->pb->error;
> +
>     return ret;
> }

Ping, I will apply this soon.

Regards,
Marton
Nicolas George Aug. 29, 2019, 8:01 p.m.
Marton Balint (12019-08-24):
> avio_feof() returns true both in case of actual EOF and in case of IO errors.
> Some demuxers (matroska) have special handling to be able to return the proper
> error for this exact reason, e.g.:
> 
> if (avio_feof(pb)) {
>      if (pb->error) {
>          return pb->error;
>      } else {
>          return AVERROR_EOF;
>      }
> }
> 
> However, most of the demuxers do not, and they simply return AVERROR_EOF if
> avio_feof() is true, so there is a real chance that IO errors are mistaken for
> EOF.
> 
> We might just say that the API user should always check the IO context error
> attribute on EOF to make sure no IO errors happened, but not even ffmpeg.c does
> this. It should be more intuitive to the API user if we simply return the IO
> error as the return value of av_read_frame() instead of AVERROR_EOF.

I am not very happy with this. It might be a short-term mitigation of
the problem, but the real issue is that demuxers do not correctly do
their error checking, ant that needs to be fixed.

I am not opposing this patch, but I think it is important to keep that
in mind.

Regards,
Marton Balint Aug. 31, 2019, 4:21 p.m.
On Thu, 29 Aug 2019, Nicolas George wrote:

> Marton Balint (12019-08-24):
>> avio_feof() returns true both in case of actual EOF and in case of IO errors.
>> Some demuxers (matroska) have special handling to be able to return the proper
>> error for this exact reason, e.g.:
>>
>> if (avio_feof(pb)) {
>>      if (pb->error) {
>>          return pb->error;
>>      } else {
>>          return AVERROR_EOF;
>>      }
>> }
>>
>> However, most of the demuxers do not, and they simply return AVERROR_EOF if
>> avio_feof() is true, so there is a real chance that IO errors are mistaken for
>> EOF.
>>
>> We might just say that the API user should always check the IO context error
>> attribute on EOF to make sure no IO errors happened, but not even ffmpeg.c does
>> this. It should be more intuitive to the API user if we simply return the IO
>> error as the return value of av_read_frame() instead of AVERROR_EOF.
>
> I am not very happy with this. It might be a short-term mitigation of
> the problem, but the real issue is that demuxers do not correctly do
> their error checking, ant that needs to be fixed.
>
> I am not opposing this patch, but I think it is important to keep that
> in mind.

Ok, applied. In the long run what do you suggest? Every demuxer should do 
the same thing as the example code I referenced in the commit message? Or 
maybe some helper functions should be introduced? The core issue is that 
it is not clear what kind of error handling we expect from the demuxers. 
As a starting point, maybe we should update the documentation first to 
cover this area.

Thanks,
Marton

Patch hide | download patch | download mbox

diff --git a/libavformat/utils.c b/libavformat/utils.c
index b57e680089..b83a740500 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -1762,6 +1762,11 @@  FF_ENABLE_DEPRECATION_WARNINGS
                av_ts2str(pkt->dts),
                pkt->size, pkt->duration, pkt->flags);
 
+    /* A demuxer might have returned EOF because of an IO error, let's
+     * propagate this back to the user. */
+    if (ret == AVERROR_EOF && s->pb && s->pb->error < 0 && s->pb->error != AVERROR(EAGAIN))
+        ret = s->pb->error;
+
     return ret;
 }