Message ID | 20211213152042.5900-21-anton@khirnov.net |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,01/24] ffmpeg: pass the muxer context explicitly to some functions | expand |
Context | Check | Description |
---|---|---|
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
andriy/make_ppc | success | Make finished |
andriy/make_fate_ppc | success | Make fate finished |
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
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++; - } - - /* 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; - } - 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); } }