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

[Andreas Rheinhardt]

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);
>          }
>      }
>