[FFmpeg-devel] [PATCH 1/2] avformat/demux: Make read_frame_internal() return AVERREOR(EAGAIN) on stuck empty input parser

[FFmpeg-devel] [PATCH 1/2] avformat/demux: Make read_frame_internal() return AVERREOR(EAGAIN) on stuck empty input parser

[Michael Niedermayer]

Fixes: read_frame_internal() which does not return even though both demuxer and parser do return
Fixes: 43717/clusterfuzz-testcase-minimized-ffmpeg_IO_DEMUXER_fuzzer-5206008287330304

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/demux.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/libavformat/demux.c b/libavformat/demux.c
index ec34b65288..dd42d32710 100644
--- a/libavformat/demux.c
+++ b/libavformat/demux.c
@@ -1222,11 +1222,15 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
     FFFormatContext *const si = ffformatcontext(s);
     int ret, got_packet = 0;
     AVDictionary *metadata = NULL;
+    int empty = 0;
 
     while (!got_packet && !si->parse_queue.head) {
         AVStream *st;
         FFStream *sti;
 
+        if (empty > 1)
+            return AVERROR(EAGAIN);
+
         /* read next packet */
         ret = ff_read_packet(s, pkt);
         if (ret < 0) {
@@ -1317,6 +1321,8 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
             }
             got_packet = 1;
         } else if (st->discard < AVDISCARD_ALL) {
+            if (pkt->size == 0)
+                empty ++;
             if ((ret = parse_packet(s, pkt, pkt->stream_index, 0)) < 0)
                 return ret;
             st->codecpar->sample_rate = sti->avctx->sample_rate;
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/2] avformat/demux: Count EAGAIN as 100 bytes in relation to read limit in avformat_find_stream_info()

[Michael Niedermayer]

Fixes: Timeout
Fixes: 43717/clusterfuzz-testcase-minimized-ffmpeg_IO_DEMUXER_fuzzer-5206008287330304

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/demux.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavformat/demux.c b/libavformat/demux.c
index dd42d32710..1acba0c608 100644
--- a/libavformat/demux.c
+++ b/libavformat/demux.c
@@ -2590,8 +2590,10 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options)
         /* NOTE: A new stream can be added there if no header in file
          * (AVFMTCTX_NOHEADER). */
         ret = read_frame_internal(ic, pkt1);
-        if (ret == AVERROR(EAGAIN))
+        if (ret == AVERROR(EAGAIN)) {
+            read_size += 100;
             continue;
+        }
 
         if (ret < 0) {
             /* EOF or error*/
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avformat/demux: Count EAGAIN as 100 bytes in relation to read limit in avformat_find_stream_info()

[Marton Balint]

On Tue, 8 Feb 2022, Michael Niedermayer wrote:

> Fixes: Timeout
> Fixes: 43717/clusterfuzz-testcase-minimized-ffmpeg_IO_DEMUXER_fuzzer-5206008287330304
>
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
> libavformat/demux.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/demux.c b/libavformat/demux.c
> index dd42d32710..1acba0c608 100644
> --- a/libavformat/demux.c
> +++ b/libavformat/demux.c
> @@ -2590,8 +2590,10 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options)
>         /* NOTE: A new stream can be added there if no header in file
>          * (AVFMTCTX_NOHEADER). */
>         ret = read_frame_internal(ic, pkt1);
> -        if (ret == AVERROR(EAGAIN))
> +        if (ret == AVERROR(EAGAIN)) {
> +            read_size += 100;

Sorry, same here, very hackish. I especially dislike that you use 
read_size for a limit because that intereferes with probesize and silently 
ignores the problem.

Can't we fix the underlying issue? If not, then counting the number of 
EAGAINs and returning a hard failure if that becomes more than e.g. 10000 
would be a lot more acceptable to me. E.g.

if (nb_egains > 10000) {
    av_log(NULL, AV_LOG_ERROR, "read_frame stuck in an EAGAIN loop, this should not happen\n);
    return AVERROR_BUG;
}

Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avformat/demux: Count EAGAIN as 100 bytes in relation to read limit in avformat_find_stream_info()

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2735 bytes --]

On Fri, Jun 17, 2022 at 10:15:39PM +0200, Marton Balint wrote:
> 
> 
> On Tue, 8 Feb 2022, Michael Niedermayer wrote:
> 
> > Fixes: Timeout
> > Fixes: 43717/clusterfuzz-testcase-minimized-ffmpeg_IO_DEMUXER_fuzzer-5206008287330304
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> > libavformat/demux.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/demux.c b/libavformat/demux.c
> > index dd42d32710..1acba0c608 100644
> > --- a/libavformat/demux.c
> > +++ b/libavformat/demux.c
> > @@ -2590,8 +2590,10 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options)
> >         /* NOTE: A new stream can be added there if no header in file
> >          * (AVFMTCTX_NOHEADER). */
> >         ret = read_frame_internal(ic, pkt1);
> > -        if (ret == AVERROR(EAGAIN))
> > +        if (ret == AVERROR(EAGAIN)) {
> > +            read_size += 100;
> 
> Sorry, same here, very hackish. I especially dislike that you use read_size
> for a limit because that intereferes with probesize and silently ignores the
> problem.
> 
> Can't we fix the underlying issue? If not, then counting the number of
> EAGAINs and returning a hard failure if that becomes more than e.g. 10000
> would be a lot more acceptable to me. E.g.
> 

> if (nb_egains > 10000) {
>    av_log(NULL, AV_LOG_ERROR, "read_frame stuck in an EAGAIN loop, this should not happen\n);
>    return AVERROR_BUG;
> }

I dont know if AVERROR_BUG is the correct return code.
Also with this the question is do you reset nb_egains after a non zero output
or not ?
if you do then you could have a case where there are 9999 empty packets 
followed by 1 byte followed by 9999 empty packets ...
if you do not then a file with 10001 1 byte packets where after each theres 1
empty one would fail
Surely these are constructed hypothetical cases but the idea of adding it
with the number of reads is so that the real "cost" is bound
maybe a sepertate nb_egains tahts non reseting could be done but with a larger
limit

I dont really have an opinion on this, i dont like mixing it in read_size
either but then i also dont like any other solution. needing an extra
threshold and potentially user setable parameter all suck too

so if you like this nb_egains code better, sure go ahead and replace it

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avformat/demux: Make read_frame_internal() return AVERREOR(EAGAIN) on stuck empty input parser

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 800 bytes --]

On Tue, Feb 08, 2022 at 09:23:52PM +0100, Michael Niedermayer wrote:
> Fixes: read_frame_internal() which does not return even though both demuxer and parser do return
> Fixes: 43717/clusterfuzz-testcase-minimized-ffmpeg_IO_DEMUXER_fuzzer-5206008287330304
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/demux.c | 6 ++++++
>  1 file changed, 6 insertions(+)

will apply patchset

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avformat/demux: Make read_frame_internal() return AVERREOR(EAGAIN) on stuck empty input parser

[Marton Balint]

On Tue, 8 Feb 2022, Michael Niedermayer wrote:

> Fixes: read_frame_internal() which does not return even though both demuxer and parser do return
> Fixes: 43717/clusterfuzz-testcase-minimized-ffmpeg_IO_DEMUXER_fuzzer-5206008287330304
>
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
> libavformat/demux.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/libavformat/demux.c b/libavformat/demux.c
> index ec34b65288..dd42d32710 100644
> --- a/libavformat/demux.c
> +++ b/libavformat/demux.c
> @@ -1222,11 +1222,15 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
>     FFFormatContext *const si = ffformatcontext(s);
>     int ret, got_packet = 0;
>     AVDictionary *metadata = NULL;
> +    int empty = 0;
>
>     while (!got_packet && !si->parse_queue.head) {
>         AVStream *st;
>         FFStream *sti;
>
> +        if (empty > 1)
> +            return AVERROR(EAGAIN);
> +
>         /* read next packet */
>         ret = ff_read_packet(s, pkt);
>         if (ret < 0) {
> @@ -1317,6 +1321,8 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
>             }
>             got_packet = 1;
>         } else if (st->discard < AVDISCARD_ALL) {
> +            if (pkt->size == 0)
> +                empty ++;
>             if ((ret = parse_packet(s, pkt, pkt->stream_index, 0)) < 0)
>                 return ret;
>             st->codecpar->sample_rate = sti->avctx->sample_rate;
> -- 
> 2.17.1

Sorry, just noticed this patchset, and it is very hackish.

For starters the meaning of AVERROR(EAGAIN) for 
av_read_frame()/read_frame_internal() is not very well defined. Should the 
user retry immediately? Should the user sleep() sometime and the retry? 
Can the user expect that a loop of av_read_frame() will eventually return 
something different than AVERROR(EAGAIN)?

I am not sure I understand the original issue entirely, but it looks 
that instead of fixing the component which returns infinite amount 
of zero sized packets you implemented a check that makes the user get an 
EAGAIN() on the second zero-sized packet.

And this helps the user how? Instead of a busy CPU loop in the library he 
either gets a busy CPU loop in its application or a non-busy CPU loop in 
its application (if he sleeps() on EAGAIN).

Is there a file which causes this? Can't the underlying components be 
fixed instead?

Thanks,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avformat/demux: Make read_frame_internal() return AVERREOR(EAGAIN) on stuck empty input parser

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 4261 bytes --]

On Fri, Jun 17, 2022 at 09:53:13PM +0200, Marton Balint wrote:
> 
> 
> On Tue, 8 Feb 2022, Michael Niedermayer wrote:
> 
> > Fixes: read_frame_internal() which does not return even though both demuxer and parser do return
> > Fixes: 43717/clusterfuzz-testcase-minimized-ffmpeg_IO_DEMUXER_fuzzer-5206008287330304
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> > libavformat/demux.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> > 
> > diff --git a/libavformat/demux.c b/libavformat/demux.c
> > index ec34b65288..dd42d32710 100644
> > --- a/libavformat/demux.c
> > +++ b/libavformat/demux.c
> > @@ -1222,11 +1222,15 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
> >     FFFormatContext *const si = ffformatcontext(s);
> >     int ret, got_packet = 0;
> >     AVDictionary *metadata = NULL;
> > +    int empty = 0;
> > 
> >     while (!got_packet && !si->parse_queue.head) {
> >         AVStream *st;
> >         FFStream *sti;
> > 
> > +        if (empty > 1)
> > +            return AVERROR(EAGAIN);
> > +
> >         /* read next packet */
> >         ret = ff_read_packet(s, pkt);
> >         if (ret < 0) {
> > @@ -1317,6 +1321,8 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
> >             }
> >             got_packet = 1;
> >         } else if (st->discard < AVDISCARD_ALL) {
> > +            if (pkt->size == 0)
> > +                empty ++;
> >             if ((ret = parse_packet(s, pkt, pkt->stream_index, 0)) < 0)
> >                 return ret;
> >             st->codecpar->sample_rate = sti->avctx->sample_rate;
> > -- 
> > 2.17.1
> 
> Sorry, just noticed this patchset, and it is very hackish.

yes thats why i waited so many month before i applied it
some patchsets i forget but this i kept pushing away

> 
> For starters the meaning of AVERROR(EAGAIN) for
> av_read_frame()/read_frame_internal() is not very well defined. Should the
> user retry immediately? Should the user sleep() sometime and the retry? Can
> the user expect that a loop of av_read_frame() will eventually return
> something different than AVERROR(EAGAIN)?

In the context of this problem the idea is to give the user an opertunity
to do something else in a single threaded environment or error out if
its taking too long not produingh anything


> 
> I am not sure I understand the original issue entirely, but it looks that
> instead of fixing the component which returns infinite amount of zero sized
> packets you implemented a check that makes the user get an EAGAIN() on the
> second zero-sized packet.

IIRC the problem was that the demuxer produced 0 sized packets and alot of them
for a file only a few hundread bytes large.
now the zero sized packets go into the parser which has no output because
theres no bytes input so no packet is ever produced by the parser and nothing
comes out of the read_frame_internal() code, its just stuck in a loop

From the point of view of the parser, the parser seems behaving correct
From the point of view of the demuxer, the demuxer seems behaving correct
if for example it produces a billion empty packets
but from the point of view of the user the code is stuck and hangs


> 
> And this helps the user how? Instead of a busy CPU loop in the library he
> either gets a busy CPU loop in its application or a non-busy CPU loop in its
> application (if he sleeps() on EAGAIN).

The advantage for the user is, she can cleanly error out instead of killing
the thread / process


> 
> Is there a file which causes this? Can't the underlying components be fixed
> instead?

i have 2 files it seems, i will mail them to you privatly
As said, its not clear if this is fundamentally any component misbehaving
maybe something does break some rule somewhere i dont know

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avformat/demux: Make read_frame_internal() return AVERREOR(EAGAIN) on stuck empty input parser

[Marton Balint]

On Sat, 18 Jun 2022, Michael Niedermayer wrote:

> On Fri, Jun 17, 2022 at 09:53:13PM +0200, Marton Balint wrote:
>>
>>
>> On Tue, 8 Feb 2022, Michael Niedermayer wrote:
>>
>>> Fixes: read_frame_internal() which does not return even though both demuxer and parser do return
>>> Fixes: 43717/clusterfuzz-testcase-minimized-ffmpeg_IO_DEMUXER_fuzzer-5206008287330304
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>> libavformat/demux.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/libavformat/demux.c b/libavformat/demux.c
>>> index ec34b65288..dd42d32710 100644
>>> --- a/libavformat/demux.c
>>> +++ b/libavformat/demux.c
>>> @@ -1222,11 +1222,15 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
>>>     FFFormatContext *const si = ffformatcontext(s);
>>>     int ret, got_packet = 0;
>>>     AVDictionary *metadata = NULL;
>>> +    int empty = 0;
>>>
>>>     while (!got_packet && !si->parse_queue.head) {
>>>         AVStream *st;
>>>         FFStream *sti;
>>>
>>> +        if (empty > 1)
>>> +            return AVERROR(EAGAIN);
>>> +
>>>         /* read next packet */
>>>         ret = ff_read_packet(s, pkt);
>>>         if (ret < 0) {
>>> @@ -1317,6 +1321,8 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
>>>             }
>>>             got_packet = 1;
>>>         } else if (st->discard < AVDISCARD_ALL) {
>>> +            if (pkt->size == 0)
>>> +                empty ++;
>>>             if ((ret = parse_packet(s, pkt, pkt->stream_index, 0)) < 0)
>>>                 return ret;
>>>             st->codecpar->sample_rate = sti->avctx->sample_rate;
>>> --
>>> 2.17.1
>>
>> Sorry, just noticed this patchset, and it is very hackish.
>
> yes thats why i waited so many month before i applied it
> some patchsets i forget but this i kept pushing away
>
>>
>> For starters the meaning of AVERROR(EAGAIN) for
>> av_read_frame()/read_frame_internal() is not very well defined. Should the
>> user retry immediately? Should the user sleep() sometime and the retry? Can
>> the user expect that a loop of av_read_frame() will eventually return
>> something different than AVERROR(EAGAIN)?
>
> In the context of this problem the idea is to give the user an opertunity
> to do something else in a single threaded environment or error out if
> its taking too long not produingh anything
>
>
>>
>> I am not sure I understand the original issue entirely, but it looks that
>> instead of fixing the component which returns infinite amount of zero sized
>> packets you implemented a check that makes the user get an EAGAIN() on the
>> second zero-sized packet.
>
> IIRC the problem was that the demuxer produced 0 sized packets and alot of them
> for a file only a few hundread bytes large.

AFAIK a demuxer is not supposed to generate 0-sized packets. Or do you 
think otherwise?

E.g. a simple patch like this seems the better approach:

--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -8677,8 +8677,12 @@ static int mov_read_packet(AVFormatContext *s, 
AVPacket *pkt)

          if (st->codecpar->codec_id == AV_CODEC_ID_EIA_608 && sample->size > 8)
              ret = get_eia608_packet(sc->pb, pkt, sample->size);
-        else
+        else if (sample->size > 0) {
              ret = av_get_packet(sc->pb, pkt, sample->size);
+        } else {
+            av_log(mov->fc, AV_LOG_ERROR, "Invalid sample size, corrupt index?\n");
+            return AVERROR_INVALIDDATA;
+        }
          if (ret < 0) {
              if (should_retry(sc->pb, ret)) {
                  mov_current_sample_dec(sc);

Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avformat/demux: Make read_frame_internal() return AVERREOR(EAGAIN) on stuck empty input parser

[Andreas Rheinhardt]

Marton Balint:
> 
> 
> On Sat, 18 Jun 2022, Michael Niedermayer wrote:
> 
>> On Fri, Jun 17, 2022 at 09:53:13PM +0200, Marton Balint wrote:
>>>
>>>
>>> On Tue, 8 Feb 2022, Michael Niedermayer wrote:
>>>
>>>> Fixes: read_frame_internal() which does not return even though both
>>>> demuxer and parser do return
>>>> Fixes:
>>>> 43717/clusterfuzz-testcase-minimized-ffmpeg_IO_DEMUXER_fuzzer-5206008287330304
>>>>
>>>>
>>>> Found-by: continuous fuzzing process
>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>> ---
>>>> libavformat/demux.c | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/libavformat/demux.c b/libavformat/demux.c
>>>> index ec34b65288..dd42d32710 100644
>>>> --- a/libavformat/demux.c
>>>> +++ b/libavformat/demux.c
>>>> @@ -1222,11 +1222,15 @@ static int
>>>> read_frame_internal(AVFormatContext *s, AVPacket *pkt)
>>>>     FFFormatContext *const si = ffformatcontext(s);
>>>>     int ret, got_packet = 0;
>>>>     AVDictionary *metadata = NULL;
>>>> +    int empty = 0;
>>>>
>>>>     while (!got_packet && !si->parse_queue.head) {
>>>>         AVStream *st;
>>>>         FFStream *sti;
>>>>
>>>> +        if (empty > 1)
>>>> +            return AVERROR(EAGAIN);
>>>> +
>>>>         /* read next packet */
>>>>         ret = ff_read_packet(s, pkt);
>>>>         if (ret < 0) {
>>>> @@ -1317,6 +1321,8 @@ static int read_frame_internal(AVFormatContext
>>>> *s, AVPacket *pkt)
>>>>             }
>>>>             got_packet = 1;
>>>>         } else if (st->discard < AVDISCARD_ALL) {
>>>> +            if (pkt->size == 0)
>>>> +                empty ++;
>>>>             if ((ret = parse_packet(s, pkt, pkt->stream_index, 0)) < 0)
>>>>                 return ret;
>>>>             st->codecpar->sample_rate = sti->avctx->sample_rate;
>>>> -- 
>>>> 2.17.1
>>>
>>> Sorry, just noticed this patchset, and it is very hackish.
>>
>> yes thats why i waited so many month before i applied it
>> some patchsets i forget but this i kept pushing away
>>
>>>
>>> For starters the meaning of AVERROR(EAGAIN) for
>>> av_read_frame()/read_frame_internal() is not very well defined.
>>> Should the
>>> user retry immediately? Should the user sleep() sometime and the
>>> retry? Can
>>> the user expect that a loop of av_read_frame() will eventually return
>>> something different than AVERROR(EAGAIN)?
>>
>> In the context of this problem the idea is to give the user an opertunity
>> to do something else in a single threaded environment or error out if
>> its taking too long not produingh anything
>>
>>
>>>
>>> I am not sure I understand the original issue entirely, but it looks
>>> that
>>> instead of fixing the component which returns infinite amount of zero
>>> sized
>>> packets you implemented a check that makes the user get an EAGAIN()
>>> on the
>>> second zero-sized packet.
>>
>> IIRC the problem was that the demuxer produced 0 sized packets and
>> alot of them
>> for a file only a few hundread bytes large.
> 
> AFAIK a demuxer is not supposed to generate 0-sized packets. Or do you
> think otherwise?

Is it not possible to use zero-sized packets to clear the screen (i.e.
convey the end timestamp of subtitles transmitted earlier)? (The
subtitle formats without duration currently use packets with a size > 0
for that, but there is no real reason why this need to be so.)
Another reason for zero-sized packets were side-data only packets (maybe
not now, but it might be used in the future; after all, we are
constantly adding new side-data-types).

(This does not mean that I am against your proposed patch.)

- Andreas

PS: The fact that the parsing API is based upon buffers and not
AVPackets btw leads to buggy side-data handling: If a parser introduces
delay, then the side-data will not be attached to the correct packet,
but to the preceding one.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avformat/demux: Make read_frame_internal() return AVERREOR(EAGAIN) on stuck empty input parser

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 699 bytes --]

On Mon, Jun 20, 2022 at 01:44:52AM +0200, Andreas Rheinhardt wrote:
[...]
> PS: The fact that the parsing API is based upon buffers and not
> AVPackets btw leads to buggy side-data handling: If a parser introduces
> delay, then the side-data will not be attached to the correct packet,
> but to the preceding one.

The parsers pass timestamps around correctly, sidedata can maybe be
handled similarly

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 2
"100% positive feedback" - "All either got their money back or didnt complain"
"Best seller ever, very honest" - "Seller refunded buyer after failed scam"

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".