[FFmpeg-devel,21/24] ffmpeg_mux: split of_write_packet()

It is currently called from two places:
- output_packet() in ffmpeg.c, which submits the newly available output
  packet to the muxer
- from of_check_init() in ffmpeg_mux.c after the header has been
  written, to flush the muxing queue

Some packets will thus be processed by this function twice, so it
requires an extra parameter to indicate the place it is called from and
avoid modifying some state twice.

This is fragile and hard to follow, so split this function into two.
Also rename of_write_packet() to of_submit_packet() to better reflect
its new purpose.
---
 fftools/ffmpeg.c     |  4 +--
 fftools/ffmpeg.h     |  3 +--
 fftools/ffmpeg_mux.c | 63 ++++++++++++++++++++++++--------------------
 3 files changed, 37 insertions(+), 33 deletions(-)

Anton Khirnov:
> It is currently called from two places:
> - output_packet() in ffmpeg.c, which submits the newly available output
>   packet to the muxer
> - from of_check_init() in ffmpeg_mux.c after the header has been
>   written, to flush the muxing queue
> 
> Some packets will thus be processed by this function twice, so it
> requires an extra parameter to indicate the place it is called from and
> avoid modifying some state twice.
> 
> This is fragile and hard to follow, so split this function into two.
> Also rename of_write_packet() to of_submit_packet() to better reflect
> its new purpose.
> ---
>  fftools/ffmpeg.c     |  4 +--
>  fftools/ffmpeg.h     |  3 +--
>  fftools/ffmpeg_mux.c | 63 ++++++++++++++++++++++++--------------------
>  3 files changed, 37 insertions(+), 33 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index e9a5c0f523..bbedf867b4 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -728,11 +728,11 @@ static void output_packet(OutputFile *of, AVPacket *pkt,
>          if (ret < 0)
>              goto finish;
>          while ((ret = av_bsf_receive_packet(ost->bsf_ctx, pkt)) >= 0)
> -            of_write_packet(of, pkt, ost, 0);
> +            of_submit_packet(of, pkt, ost);
>          if (ret == AVERROR(EAGAIN))
>              ret = 0;
>      } else if (!eof)
> -        of_write_packet(of, pkt, ost, 0);
> +        of_submit_packet(of, pkt, ost);
>  
>  finish:
>      if (ret < 0 && ret != AVERROR_EOF) {
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index e6e472f994..76c8dfa4c8 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -684,8 +684,7 @@ int of_check_init(OutputFile *of);
>  int of_write_trailer(OutputFile *of);
>  void of_close(OutputFile **pof);
>  
> -void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
> -                     int unqueue);
> +void of_submit_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost);
>  int of_finished(OutputFile *of);
>  int64_t of_bytes_written(OutputFile *of);
>  AVChapter * const *
> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> index d4b674c9e2..e97ec8ab93 100644
> --- a/fftools/ffmpeg_mux.c
> +++ b/fftools/ffmpeg_mux.c
> @@ -102,39 +102,12 @@ static int queue_packet(OutputFile *of, OutputStream *ost, AVPacket *pkt)
>      return 0;
>  }
>  
> -void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
> -                     int unqueue)
> +static void write_packet(OutputFile *of, OutputStream *ost, AVPacket *pkt)
>  {
>      AVFormatContext *s = of->ctx;
>      AVStream *st = ost->st;
>      int ret;
>  
> -    /*
> -     * Audio encoders may split the packets --  #frames in != #packets out.
> -     * But there is no reordering, so we can limit the number of output packets
> -     * by simply dropping them here.
> -     * Counting encoded video frames needs to be done separately because of
> -     * reordering, see do_video_out().
> -     * Do not count the packet when unqueued because it has been counted when queued.
> -     */
> -    if (!(st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && ost->encoding_needed) && !unqueue) {
> -        if (ost->frame_number >= ost->max_frames) {
> -            av_packet_unref(pkt);
> -            return;
> -        }
> -        ost->frame_number++;
> -    }

Factoring this chunk out of write_packet() (effectively inlining
unqueue) looks good (and I actually pondered it myself),

> -
> -    /* the muxer is not initialized yet, buffer the packet */
> -    if (!of->mux->header_written) {
> -        ret = queue_packet(of, ost, pkt);
> -        if (ret < 0) {
> -            av_packet_unref(pkt);
> -            exit_program(1);
> -        }
> -        return;
> -    }
> -

but I could not prove that the header has already been written in case
unqueue == 0. Can you guarantee this to be so and explain it to me?

>      if ((st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && video_sync_method == VSYNC_DROP) ||
>          (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && audio_sync_method < 0))
>          pkt->pts = pkt->dts = AV_NOPTS_VALUE;
> @@ -225,6 +198,38 @@ void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
>      }
>  }
>  
> +void of_submit_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost)
> +{
> +    AVStream *st = ost->st;
> +    int ret;
> +
> +    /*
> +     * Audio encoders may split the packets --  #frames in != #packets out.
> +     * But there is no reordering, so we can limit the number of output packets
> +     * by simply dropping them here.
> +     * Counting encoded video frames needs to be done separately because of
> +     * reordering, see do_video_out().
> +     */
> +    if (!(st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && ost->encoding_needed)) {
> +        if (ost->frame_number >= ost->max_frames) {
> +            av_packet_unref(pkt);
> +            return;
> +        }
> +        ost->frame_number++;
> +    }
> +
> +    if (of->mux->header_written) {
> +        write_packet(of, ost, pkt);
> +    } else {
> +        /* the muxer is not initialized yet, buffer the packet */
> +        ret = queue_packet(of, ost, pkt);
> +        if (ret < 0) {
> +            av_packet_unref(pkt);
> +            exit_program(1);
> +        }
> +    }
> +}
> +
>  static int print_sdp(void)
>  {
>      char sdp[16384];
> @@ -324,7 +329,7 @@ int of_check_init(OutputFile *of)
>              AVPacket *pkt;
>              av_fifo_generic_read(ms->muxing_queue, &pkt, sizeof(pkt), NULL);
>              ms->muxing_queue_data_size -= pkt->size;
> -            of_write_packet(of, pkt, ost, 1);
> +            write_packet(of, ost, pkt);
>              av_packet_free(&pkt);
>          }
>      }
>

Quoting Andreas Rheinhardt (2021-12-17 00:42:25)
> Anton Khirnov:
> > diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> > index d4b674c9e2..e97ec8ab93 100644
> > --- a/fftools/ffmpeg_mux.c
> > +++ b/fftools/ffmpeg_mux.c
> > @@ -102,39 +102,12 @@ static int queue_packet(OutputFile *of, OutputStream *ost, AVPacket *pkt)
> >      return 0;
> >  }
> >  
> > -void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
> > -                     int unqueue)
> > +static void write_packet(OutputFile *of, OutputStream *ost, AVPacket *pkt)
> >  {
> >      AVFormatContext *s = of->ctx;
> >      AVStream *st = ost->st;
> >      int ret;
> >  
> > -    /*
> > -     * Audio encoders may split the packets --  #frames in != #packets out.
> > -     * But there is no reordering, so we can limit the number of output packets
> > -     * by simply dropping them here.
> > -     * Counting encoded video frames needs to be done separately because of
> > -     * reordering, see do_video_out().
> > -     * Do not count the packet when unqueued because it has been counted when queued.
> > -     */
> > -    if (!(st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && ost->encoding_needed) && !unqueue) {
> > -        if (ost->frame_number >= ost->max_frames) {
> > -            av_packet_unref(pkt);
> > -            return;
> > -        }
> > -        ost->frame_number++;
> > -    }
> 
> Factoring this chunk out of write_packet() (effectively inlining
> unqueue) looks good (and I actually pondered it myself),
> 
> > -
> > -    /* the muxer is not initialized yet, buffer the packet */
> > -    if (!of->mux->header_written) {
> > -        ret = queue_packet(of, ost, pkt);
> > -        if (ret < 0) {
> > -            av_packet_unref(pkt);
> > -            exit_program(1);
> > -        }
> > -        return;
> > -    }
> > -
> 
> but I could not prove that the header has already been written in case
> unqueue == 0. Can you guarantee this to be so and explain it to me?

I don't understand what you mean. unqueue == 0 in the current code tells
you nothing about whether the header has been written.
How does that relate to the patch?

Anton Khirnov:
> Quoting Andreas Rheinhardt (2021-12-17 00:42:25)
>> Anton Khirnov:
>>> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
>>> index d4b674c9e2..e97ec8ab93 100644
>>> --- a/fftools/ffmpeg_mux.c
>>> +++ b/fftools/ffmpeg_mux.c
>>> @@ -102,39 +102,12 @@ static int queue_packet(OutputFile *of, OutputStream *ost, AVPacket *pkt)
>>>      return 0;
>>>  }
>>>  
>>> -void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
>>> -                     int unqueue)
>>> +static void write_packet(OutputFile *of, OutputStream *ost, AVPacket *pkt)
>>>  {
>>>      AVFormatContext *s = of->ctx;
>>>      AVStream *st = ost->st;
>>>      int ret;
>>>  
>>> -    /*
>>> -     * Audio encoders may split the packets --  #frames in != #packets out.
>>> -     * But there is no reordering, so we can limit the number of output packets
>>> -     * by simply dropping them here.
>>> -     * Counting encoded video frames needs to be done separately because of
>>> -     * reordering, see do_video_out().
>>> -     * Do not count the packet when unqueued because it has been counted when queued.
>>> -     */
>>> -    if (!(st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && ost->encoding_needed) && !unqueue) {
>>> -        if (ost->frame_number >= ost->max_frames) {
>>> -            av_packet_unref(pkt);
>>> -            return;
>>> -        }
>>> -        ost->frame_number++;
>>> -    }
>>
>> Factoring this chunk out of write_packet() (effectively inlining
>> unqueue) looks good (and I actually pondered it myself),
>>
>>> -
>>> -    /* the muxer is not initialized yet, buffer the packet */
>>> -    if (!of->mux->header_written) {
>>> -        ret = queue_packet(of, ost, pkt);
>>> -        if (ret < 0) {
>>> -            av_packet_unref(pkt);
>>> -            exit_program(1);
>>> -        }
>>> -        return;
>>> -    }
>>> -
>>
>> but I could not prove that the header has already been written in case
>> unqueue == 0. Can you guarantee this to be so and explain it to me?
> 
> I don't understand what you mean. unqueue == 0 in the current code tells
> you nothing about whether the header has been written.
> How does that relate to the patch?
> 

Wait, I misread this: You keep the header_written check for the two
callers in output_packet(), but remove it for the one caller for which
we know that initialization has already happened. This is good. Sorry
for the confusion.

- Andreas