[FFmpeg-devel] [PATCH 1/2] avformat/matroskaenc: Don't create wrong packet durations
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Sun Oct 1 16:08:51 EEST 2023
Andreas Rheinhardt:
> We have to write an explicit BlockDuration element (and use
> a BlockGroup instead of a SimpleBlock) in case the Track
> has a DefaultDuration that is inconsistent with the duration
> of the packet.
>
> The matroska-h264-remux test uses a file with coded fields
> where the duration of a Block is the duration of a field,
> not of a frame, therefore this patch writes said BlockDuration
> elements.
>
> (When using a BlockGroup, one has to add ReferenceBlock elements
> to distinguish keyframes from non-keyframes. Unfortunately,
> the AV1 codec mapping [1] requires us to reference all references
> and to really use the real references, which requires a lot of
> effort for basically no gain. When BlockGroups are used with AV1,
> the created files are most likely invalid, both before and after
> this patch, but this patch makes this more likely to happen.)
>
> [1]: https://github.com/ietf-wg-cellar/matroska-specification/blob/master/codec/av1.md
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> ---
> libavformat/matroskaenc.c | 40 ++++++++++++++++++++++--------
> tests/ref/fate/matroska-h264-remux | 4 +--
> 2 files changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index ba54f5f98e..9286932a08 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -195,6 +195,8 @@ typedef struct mkv_track {
> int codecpriv_offset;
> unsigned codecpriv_size; ///< size reserved for CodecPrivate excluding header+length field
> int64_t ts_offset;
> + uint64_t default_duration_low;
> + uint64_t default_duration_high;
> /* This callback will be called twice: First with a NULL AVIOContext
> * to return the size of the (Simple)Block's data via size
> * and a second time with the AVIOContext set when the data
> @@ -1805,6 +1807,16 @@ static int mkv_write_track_video(AVFormatContext *s, MatroskaMuxContext *mkv,
> return ebml_writer_write(&writer, pb);
> }
>
> +static void mkv_write_default_duration(mkv_track *track, AVIOContext *pb,
> + AVRational duration)
> +{
> + put_ebml_uint(pb, MATROSKA_ID_TRACKDEFAULTDURATION,
> + 1000000000LL * duration.num / duration.den);
> + track->default_duration_low = 1000LL * duration.num / duration.den;
> + track->default_duration_high = track->default_duration_low +
> + !!(1000LL * duration.num % duration.den);
> +}
> +
> static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
> AVStream *st, mkv_track *track, AVIOContext *pb,
> int is_default)
> @@ -1913,16 +1925,19 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
> }
>
> switch (par->codec_type) {
> + AVRational frame_rate;
> case AVMEDIA_TYPE_VIDEO:
> mkv->have_video = 1;
> put_ebml_uint(pb, MATROSKA_ID_TRACKTYPE, MATROSKA_TRACK_TYPE_VIDEO);
>
> - if( st->avg_frame_rate.num > 0 && st->avg_frame_rate.den > 0
> - && av_cmp_q(av_inv_q(st->avg_frame_rate), st->time_base) > 0)
> - put_ebml_uint(pb, MATROSKA_ID_TRACKDEFAULTDURATION, 1000000000LL * st->avg_frame_rate.den / st->avg_frame_rate.num);
> - else if( st->r_frame_rate.num > 0 && st->r_frame_rate.den > 0
> - && av_cmp_q(av_inv_q(st->r_frame_rate), st->time_base) > 0)
> - put_ebml_uint(pb, MATROSKA_ID_TRACKDEFAULTDURATION, 1000000000LL * st->r_frame_rate.den / st->r_frame_rate.num);
> + frame_rate = (AVRational){ 0, 1 };
> + if (st->avg_frame_rate.num > 0 && st->avg_frame_rate.den > 0)
> + frame_rate = st->avg_frame_rate;
> + else if (st->r_frame_rate.num > 0 && st->r_frame_rate.den > 0)
> + frame_rate = st->r_frame_rate;
> +
> + if (frame_rate.num > 0)
> + mkv_write_default_duration(track, pb, av_inv_q(frame_rate));
>
> if (CONFIG_MATROSKA_MUXER && !native_id &&
> ff_codec_get_tag(ff_codec_movvideo_tags, par->codec_id) &&
> @@ -2739,7 +2754,12 @@ static int mkv_write_block(void *logctx, MatroskaMuxContext *mkv,
> ebml_writer_open_master(&writer, MATROSKA_ID_BLOCKGROUP);
> ebml_writer_add_block(&writer, mkv);
>
> - if (duration)
> + if (duration > 0 && (par->codec_type == AVMEDIA_TYPE_SUBTITLE ||
> + /* If the packet's duration is inconsistent with the default duration,
> + * add an explicit duration element. */
> + track->default_duration_high > 0 &&
> + duration != track->default_duration_high &&
> + duration != track->default_duration_low))
> ebml_writer_add_uint(&writer, MATROSKA_ID_BLOCKDURATION, duration);
>
> av_log(logctx, AV_LOG_DEBUG,
> @@ -2917,7 +2937,7 @@ static int mkv_write_packet_internal(AVFormatContext *s, const AVPacket *pkt)
> /* All subtitle blocks are considered to be keyframes. */
> int keyframe = is_sub || !!(pkt->flags & AV_PKT_FLAG_KEY);
> int64_t duration = FFMAX(pkt->duration, 0);
> - int64_t write_duration = is_sub ? duration : 0;
> + int64_t cue_duration = is_sub ? duration : 0;
> int ret;
> int64_t ts = track->write_dts ? pkt->dts : pkt->pts;
> int64_t relative_packet_pos;
> @@ -2958,7 +2978,7 @@ static int mkv_write_packet_internal(AVFormatContext *s, const AVPacket *pkt)
> /* The WebM spec requires WebVTT to be muxed in BlockGroups;
> * so we force it even for packets without duration. */
> ret = mkv_write_block(s, mkv, pb, par, track, pkt,
> - keyframe, ts, write_duration,
> + keyframe, ts, duration,
> par->codec_id == AV_CODEC_ID_WEBVTT,
> relative_packet_pos);
> if (ret < 0)
> @@ -2969,7 +2989,7 @@ static int mkv_write_packet_internal(AVFormatContext *s, const AVPacket *pkt)
> !mkv->have_video && !track->has_cue)) {
> ret = mkv_add_cuepoint(mkv, pkt->stream_index, ts,
> mkv->cluster_pos, relative_packet_pos,
> - write_duration);
> + cue_duration);
> if (ret < 0)
> return ret;
> track->has_cue = 1;
> diff --git a/tests/ref/fate/matroska-h264-remux b/tests/ref/fate/matroska-h264-remux
> index 2c727f03cd..6362b6f684 100644
> --- a/tests/ref/fate/matroska-h264-remux
> +++ b/tests/ref/fate/matroska-h264-remux
> @@ -1,5 +1,5 @@
> -f6b943ed3ff05087d0ef58fbaf7abcb0 *tests/data/fate/matroska-h264-remux.matroska
> -2036067 tests/data/fate/matroska-h264-remux.matroska
> +277a08f708965112a7c2fb25d941e68a *tests/data/fate/matroska-h264-remux.matroska
> +2036806 tests/data/fate/matroska-h264-remux.matroska
> #tb 0: 1/25
> #media_type 0: video
> #codec_id 0: rawvideo
Will apply this patchset tomorrow unless there are objections.
- Andreas
More information about the ffmpeg-devel
mailing list