| Message ID | 20230204100121.6815-1-ffmpeg@gyani.pro |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] ffmpeg_mux: terminate stream thread queue only on sq_send EOF | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Quoting Gyan Doshi (2023-02-04 11:01:21) > Prior to 2d924b3a630, ffmpeg would exit if any packet write failed. > After the switch to threaded mode for muxing, ffmpeg only closes that > OutputStream instead of closng the file. This happens because EOF > returned by write_packet isn't distinguished from EOF returned by sq_send, > both relayed via sync_queue_process. > > This breaks the abort behaviour when there are multiple streams in an output, > and can leave the ffmpeg process running beyond the intended point of abortion. > > Fixed by marking the OutputStream as finished upon sq_send EOF and letting > write_packet EOF lead to muxer termination. What is the situation you're handling exactly? av_interleaved_write_frame() returns EOF?
On 2023-02-07 01:39 pm, Anton Khirnov wrote: > Quoting Gyan Doshi (2023-02-04 11:01:21) >> Prior to 2d924b3a630, ffmpeg would exit if any packet write failed. >> After the switch to threaded mode for muxing, ffmpeg only closes that >> OutputStream instead of closng the file. This happens because EOF >> returned by write_packet isn't distinguished from EOF returned by sq_send, >> both relayed via sync_queue_process. >> >> This breaks the abort behaviour when there are multiple streams in an output, >> and can leave the ffmpeg process running beyond the intended point of abortion. >> >> Fixed by marking the OutputStream as finished upon sq_send EOF and letting >> write_packet EOF lead to muxer termination. > What is the situation you're handling exactly? > av_interleaved_write_frame() returns EOF? Yes. Regards, Gyan
Quoting Gyan Doshi (2023-02-07 09:18:22) > > > On 2023-02-07 01:39 pm, Anton Khirnov wrote: > > Quoting Gyan Doshi (2023-02-04 11:01:21) > >> Prior to 2d924b3a630, ffmpeg would exit if any packet write failed. > >> After the switch to threaded mode for muxing, ffmpeg only closes that > >> OutputStream instead of closng the file. This happens because EOF > >> returned by write_packet isn't distinguished from EOF returned by sq_send, > >> both relayed via sync_queue_process. > >> > >> This breaks the abort behaviour when there are multiple streams in an output, > >> and can leave the ffmpeg process running beyond the intended point of abortion. > >> > >> Fixed by marking the OutputStream as finished upon sq_send EOF and letting > >> write_packet EOF lead to muxer termination. > > What is the situation you're handling exactly? > > av_interleaved_write_frame() returns EOF? > > Yes. How does that happen? Doesn't seem to me that muxers should do this. Otherwise, I'm not a big fan of your patch since it adds yet more overloading to ost->finished, which I'd like to remove actually. Not to mention you manipulate it from the muxer thread, which is a race. IMO this should be done without any context variables.
On 2023-02-07 02:03 pm, Anton Khirnov wrote: > Quoting Gyan Doshi (2023-02-07 09:18:22) >> >> On 2023-02-07 01:39 pm, Anton Khirnov wrote: >>> Quoting Gyan Doshi (2023-02-04 11:01:21) >>>> Prior to 2d924b3a630, ffmpeg would exit if any packet write failed. >>>> After the switch to threaded mode for muxing, ffmpeg only closes that >>>> OutputStream instead of closng the file. This happens because EOF >>>> returned by write_packet isn't distinguished from EOF returned by sq_send, >>>> both relayed via sync_queue_process. >>>> >>>> This breaks the abort behaviour when there are multiple streams in an output, >>>> and can leave the ffmpeg process running beyond the intended point of abortion. >>>> >>>> Fixed by marking the OutputStream as finished upon sq_send EOF and letting >>>> write_packet EOF lead to muxer termination. >>> What is the situation you're handling exactly? >>> av_interleaved_write_frame() returns EOF? >> Yes. > How does that happen? Doesn't seem to me that muxers should do this. For repro, see this: https://ffmpeg.org/pipermail/ffmpeg-devel/2023-February/306247.html > Otherwise, I'm not a big fan of your patch since it adds yet more > overloading to ost->finished, which I'd like to remove actually. Not to > mention you manipulate it from the muxer thread, which is a race. > IMO this should be done without any context variables. How would one do that, in this case? I first thought of checking SyncQueue->stream[i]->finished but SyncQueueStream's declaration is private. Regards, Gyan
diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c index 5d427b44ea..b40a2d01a7 100644 --- a/fftools/ffmpeg_mux.c +++ b/fftools/ffmpeg_mux.c @@ -160,8 +160,12 @@ static int sync_queue_process(Muxer *mux, OutputStream *ost, AVPacket *pkt) if (ost->sq_idx_mux >= 0) { int ret = sq_send(mux->sq_mux, ost->sq_idx_mux, SQPKT(pkt)); - if (ret < 0) + if (ret < 0) { + if (ret == AVERROR_EOF) + ost->finished |= ENCODER_FINISHED; return ret; + } + while (1) { ret = sq_receive(mux->sq_mux, -1, SQPKT(mux->sq_pkt)); @@ -215,7 +219,7 @@ static void *muxer_thread(void *arg) ost = of->streams[stream_idx]; ret = sync_queue_process(mux, ost, ret < 0 ? NULL : pkt); av_packet_unref(pkt); - if (ret == AVERROR_EOF) + if (ret == AVERROR_EOF && ost->finished) tq_receive_finish(mux->tq, stream_idx); else if (ret < 0) { av_log(mux, AV_LOG_ERROR, "Error muxing a packet\n");