Re: [FFmpeg-devel] [PATCH 1/2] avformat/matroskaenc: Don't create wrong packet durations

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".