| Message ID | 20221108112550.8375-2-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/5] lavf/rtpdec_asf: set AVFMT_FLAG_NONBLOCK | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/configure_loongarch64 | warning | Failed to run configure |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Tue, Nov 08, 2022 at 12:25:47PM +0100, Anton Khirnov wrote: > Lavf only supports a very limited approximation of non-blocking > behavior, so we should not return random EAGAINs to callers unless they > specifically requested it. > --- > libavformat/demux.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) this changes seeking behavior of for example: libavformat/tests/seek fate-suite/dss/sp.dss (this seemed to return EAGAIN before sometimes) thx [...]
Quoting Michael Niedermayer (2022-11-10 13:28:27) > On Tue, Nov 08, 2022 at 12:25:47PM +0100, Anton Khirnov wrote: > > Lavf only supports a very limited approximation of non-blocking > > behavior, so we should not return random EAGAINs to callers unless they > > specifically requested it. > > --- > > libavformat/demux.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > this changes seeking behavior of for example: > libavformat/tests/seek fate-suite/dss/sp.dss > > (this seemed to return EAGAIN before sometimes) Are you saying that is wrong? I don't see why seeks should return EAGAIN either.
On 11/10/22, Anton Khirnov <anton@khirnov.net> wrote: > Quoting Michael Niedermayer (2022-11-10 13:28:27) >> On Tue, Nov 08, 2022 at 12:25:47PM +0100, Anton Khirnov wrote: >> > Lavf only supports a very limited approximation of non-blocking >> > behavior, so we should not return random EAGAINs to callers unless they >> > specifically requested it. >> > --- >> > libavformat/demux.c | 8 ++++++-- >> > 1 file changed, 6 insertions(+), 2 deletions(-) >> >> this changes seeking behavior of for example: >> libavformat/tests/seek fate-suite/dss/sp.dss >> >> (this seemed to return EAGAIN before sometimes) > > Are you saying that is wrong? I don't see why seeks should return EAGAIN > either. > It returns internally when passing seeking to generic seeking. Please do not break seeking in demuxers. > -- > Anton Khirnov > _______________________________________________ > 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". >
Quoting Paul B Mahol (2022-11-10 13:52:52) > On 11/10/22, Anton Khirnov <anton@khirnov.net> wrote: > > Quoting Michael Niedermayer (2022-11-10 13:28:27) > >> On Tue, Nov 08, 2022 at 12:25:47PM +0100, Anton Khirnov wrote: > >> > Lavf only supports a very limited approximation of non-blocking > >> > behavior, so we should not return random EAGAINs to callers unless they > >> > specifically requested it. > >> > --- > >> > libavformat/demux.c | 8 ++++++-- > >> > 1 file changed, 6 insertions(+), 2 deletions(-) > >> > >> this changes seeking behavior of for example: > >> libavformat/tests/seek fate-suite/dss/sp.dss > >> > >> (this seemed to return EAGAIN before sometimes) > > > > Are you saying that is wrong? I don't see why seeks should return EAGAIN > > either. > > > > It returns internally when passing seeking to generic seeking. > Please do not break seeking in demuxers. What exact use case is being broken by this?
On Thu, Nov 10, 2022 at 01:31:44PM +0100, Anton Khirnov wrote: > Quoting Michael Niedermayer (2022-11-10 13:28:27) > > On Tue, Nov 08, 2022 at 12:25:47PM +0100, Anton Khirnov wrote: > > > Lavf only supports a very limited approximation of non-blocking > > > behavior, so we should not return random EAGAINs to callers unless they > > > specifically requested it. > > > --- > > > libavformat/demux.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > this changes seeking behavior of for example: > > libavformat/tests/seek fate-suite/dss/sp.dss > > > > (this seemed to return EAGAIN before sometimes) > > Are you saying that is wrong? I don't see why seeks should return EAGAIN > either. I do not know if its wrong or not. I remember some seeking used error returns to switch to generic seeking and such stuff. I just reporrted this as i saw a difference in the tests/seek output thx [...]
Quoting Michael Niedermayer (2022-11-10 20:52:33) > On Thu, Nov 10, 2022 at 01:31:44PM +0100, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2022-11-10 13:28:27) > > > On Tue, Nov 08, 2022 at 12:25:47PM +0100, Anton Khirnov wrote: > > > > Lavf only supports a very limited approximation of non-blocking > > > > behavior, so we should not return random EAGAINs to callers unless they > > > > specifically requested it. > > > > --- > > > > libavformat/demux.c | 8 ++++++-- > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > this changes seeking behavior of for example: > > > libavformat/tests/seek fate-suite/dss/sp.dss > > > > > > (this seemed to return EAGAIN before sometimes) > > > > Are you saying that is wrong? I don't see why seeks should return EAGAIN > > either. > > I do not know if its wrong or not. I remember some seeking used error > returns to switch to generic seeking and such stuff. > I just reporrted this as i saw a difference in the tests/seek output AFAICT the differences are only in av_read_frame() calls, which no longer return EAGAIN. That is exactly what this patch is supposed to do, so I'd say there is no problem. If noone has further objections, I'll push this patch soonish.
Anton Khirnov (12022-11-14):
> If noone has further objections, I'll push this patch soonish.
You will do no scu thing since it is bogus, as I explained.
Quoting Nicolas George (2022-11-14 11:44:55) > Anton Khirnov (12022-11-14): > > If noone has further objections, I'll push this patch soonish. > > You will do no scu thing since it is bogus, as I explained. I do not see any objections from you to this patch.
Anton Khirnov (12022-11-14):
> I do not see any objections from you to this patch.
My explanations for the other patch apply to this one, obviously: if you
treat EAGAIN as REDO, you introduce a busy wait.
Quoting Nicolas George (2022-11-14 12:05:10) > Anton Khirnov (12022-11-14): > > I do not see any objections from you to this patch. > > My explanations for the other patch apply to this one, obviously: if you > treat EAGAIN as REDO, you introduce a busy wait. In what specific case would this supposed busy wait happen and why is it wrong?
Anton Khirnov (12022-11-14): > In what specific case would this supposed busy wait happen and why is it > wrong? See my mail from 9 Nov 2022 13:42:24 +0100 for explanations about the semantic of EAGAIN.
Quoting Nicolas George (2022-11-14 13:40:35) > Anton Khirnov (12022-11-14): > > In what specific case would this supposed busy wait happen and why is it > > wrong? > > See my mail from 9 Nov 2022 13:42:24 +0100 for explanations about the > semantic of EAGAIN. You wrote a whole lot of text and I don't see how any of it applies to this patch. The standard convention everywhere is that IO may return EAGAIN in non-blocking mode and blocks otherwise. And that is exactly what this patch implements.
Anton Khirnov (12022-11-14): > You wrote a whole lot of text and I don't see how any of it applies to > this patch. > > The standard convention everywhere is that IO may return EAGAIN in > non-blocking mode and blocks otherwise. And that is exactly what this > patch implements. To implement blocking on top of EAGAIN, a sleep is necessary. REDO must not sleep.
diff --git a/libavformat/demux.c b/libavformat/demux.c index 2dfd82a63c..5cd9522367 100644 --- a/libavformat/demux.c +++ b/libavformat/demux.c @@ -574,8 +574,12 @@ FF_ENABLE_DEPRECATION_WARNINGS /* Some demuxers return FFERROR_REDO when they consume data and discard it (ignored streams, junk, extradata). - We must re-call the demuxer to get the real packet. */ - if (err == FFERROR_REDO) + We must re-call the demuxer to get the real packet. + + Treat EAGAIN the same as FFERROR_REDO, unless the user + requested non-blocking behavior. */ + if (err == FFERROR_REDO || + (err == AVERROR(EAGAIN) && !(s->flags & AVFMT_FLAG_NONBLOCK))) continue; if (!pktl || err == AVERROR(EAGAIN)) return err;