| Message ID | AS8P250MB0744A27B21CEC9E7014FED7C8FC0A@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | 0c1103d4dc4c6af56b09df6d4ecb252606666d7b |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avformat/matroskaenc: Don't create wrong packet durations | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 9/29/2023 1:01 PM, Andreas Rheinhardt wrote: > 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.) AV1 has no fields, and the vast majority of samples will also have fixed frame duration across the entire coded stream, so i doubt this will make a difference. There's also the fact it's unlikely parsers will care about ReferenceBlock elements at all to begin with, and instead just have the decoder handle things relying on the bitstream level information. > > [1]: https://github.com/ietf-wg-cellar/matroska-specification/blob/master/codec/av1.md > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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
James Almer: > On 9/29/2023 1:01 PM, Andreas Rheinhardt wrote: >> 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.) > > AV1 has no fields, and the vast majority of samples will also have fixed > frame duration across the entire coded stream, so i doubt this will make > a difference. > > There's also the fact it's unlikely parsers will care about > ReferenceBlock elements at all to begin with, and instead just have the > decoder handle things relying on the bitstream level information. > That's why I consider this requirement to be nonsense. In fact, I would only require one thing from the ReferenceBlock elements: If one recursively goes back the ReferenceBlock elements until one has reached a keyframe, then decoding these blocks is sufficient to be able to decode the actually desired block. That way a simple muxer like ours could always reference the preceding block (in case the current packet is not a keyframe) and create a valid stream. - Andreas
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@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
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
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@outlook.com> --- libavformat/matroskaenc.c | 40 ++++++++++++++++++++++-------- tests/ref/fate/matroska-h264-remux | 4 +-- 2 files changed, 32 insertions(+), 12 deletions(-)