[FFmpeg-devel] [PATCH] ffmpeg_mux: terminate stream thread queue only on sq_send EOF

[FFmpeg-devel] [PATCH] ffmpeg_mux: terminate stream thread queue only on sq_send EOF

[Gyan Doshi]

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.
---
 fftools/ffmpeg_mux.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

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");
-- 
2.39.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] ffmpeg_mux: terminate stream thread queue only on sq_send EOF

[Anton Khirnov]

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?

-- 
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".

Re: [FFmpeg-devel] [PATCH] ffmpeg_mux: terminate stream thread queue only on sq_send EOF

[Gyan Doshi]

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

_______________________________________________
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] ffmpeg_mux: terminate stream thread queue only on sq_send EOF

[Anton Khirnov]

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.

-- 
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".

Re: [FFmpeg-devel] [PATCH] ffmpeg_mux: terminate stream thread queue only on sq_send EOF

[Gyan Doshi]

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

_______________________________________________
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] fftools/ffmpeg_mux: distinguish between sync queue and muxer EOF

[Anton Khirnov]

---
How about something like this?
---
 fftools/ffmpeg_mux.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
index dffc1410c8..cf58051949 100644
--- a/fftools/ffmpeg_mux.c
+++ b/fftools/ffmpeg_mux.c
@@ -159,14 +159,18 @@ fail:
     return ret;
 }
 
-static int sync_queue_process(Muxer *mux, OutputStream *ost, AVPacket *pkt)
+static int sync_queue_process(Muxer *mux, OutputStream *ost, AVPacket *pkt, int *stream_eof)
 {
     OutputFile *of = &mux->of;
 
     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)
+                *stream_eof = 1;
+
             return ret;
+        }
 
         while (1) {
             ret = sq_receive(mux->sq_mux, -1, SQPKT(mux->sq_pkt));
@@ -208,7 +212,7 @@ static void *muxer_thread(void *arg)
 
     while (1) {
         OutputStream *ost;
-        int stream_idx;
+        int stream_idx, stream_eof = 0;
 
         ret = tq_receive(mux->tq, &stream_idx, pkt);
         if (stream_idx < 0) {
@@ -218,9 +222,9 @@ static void *muxer_thread(void *arg)
         }
 
         ost = of->streams[stream_idx];
-        ret = sync_queue_process(mux, ost, ret < 0 ? NULL : pkt);
+        ret = sync_queue_process(mux, ost, ret < 0 ? NULL : pkt, &stream_eof);
         av_packet_unref(pkt);
-        if (ret == AVERROR_EOF)
+        if (ret == AVERROR_EOF && stream_eof)
             tq_receive_finish(mux->tq, stream_idx);
         else if (ret < 0) {
             av_log(mux, AV_LOG_ERROR, "Error muxing a packet\n");
-- 
2.35.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] fftools/ffmpeg_mux: distinguish between sync queue and muxer EOF

[Gyan Doshi]

On 2023-02-09 09:27 pm, Anton Khirnov wrote:
> ---
> How about something like this?

Yes, this works.

Regards,
Gyan

> ---
>   fftools/ffmpeg_mux.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> index dffc1410c8..cf58051949 100644
> --- a/fftools/ffmpeg_mux.c
> +++ b/fftools/ffmpeg_mux.c
> @@ -159,14 +159,18 @@ fail:
>       return ret;
>   }
>   
> -static int sync_queue_process(Muxer *mux, OutputStream *ost, AVPacket *pkt)
> +static int sync_queue_process(Muxer *mux, OutputStream *ost, AVPacket *pkt, int *stream_eof)
>   {
>       OutputFile *of = &mux->of;
>   
>       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)
> +                *stream_eof = 1;
> +
>               return ret;
> +        }
>   
>           while (1) {
>               ret = sq_receive(mux->sq_mux, -1, SQPKT(mux->sq_pkt));
> @@ -208,7 +212,7 @@ static void *muxer_thread(void *arg)
>   
>       while (1) {
>           OutputStream *ost;
> -        int stream_idx;
> +        int stream_idx, stream_eof = 0;
>   
>           ret = tq_receive(mux->tq, &stream_idx, pkt);
>           if (stream_idx < 0) {
> @@ -218,9 +222,9 @@ static void *muxer_thread(void *arg)
>           }
>   
>           ost = of->streams[stream_idx];
> -        ret = sync_queue_process(mux, ost, ret < 0 ? NULL : pkt);
> +        ret = sync_queue_process(mux, ost, ret < 0 ? NULL : pkt, &stream_eof);
>           av_packet_unref(pkt);
> -        if (ret == AVERROR_EOF)
> +        if (ret == AVERROR_EOF && stream_eof)
>               tq_receive_finish(mux->tq, stream_idx);
>           else if (ret < 0) {
>               av_log(mux, AV_LOG_ERROR, "Error muxing a packet\n");

_______________________________________________
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".