[FFmpeg-devel] [PATCH] matroskadec, matroskadec, srtenc: Read/Write duration for subtitles.

[Alexander Strasser]

Hi,

Philip Langdale wrote:
> Ok, let's get this sorted out once and for all.
> 
> I can't see any reason to manipulate the convergence duration instead.
> These subtitle packets are the moral equivalent of keyframes, and
> while the documentation says it should be used for 'some types of
> subtitles', I don't see how Matroska or srt text subtitles fall into
> that category.
> 
> The original claim was that convergence_duration was needed to avoid
> overflow on long duration subtitles. This claim seems questionable.
> If we consider the typical timebase of 1/1000, that still allows
> for a duration of 8 years. For a 1/1000000 timebase, you still get
> a 71 minute maximum duration, so I'm not sure where this claim
> originated from. Only if you go down to a 1ns timebase do you end
> up with a short max duration of ~4 seconds. Am I missing something?
> 
> This change is backward compatible by reading and writing
> convergence_duration. I don't know if that's really necessary.

  I am in favor of your patch.

  Just two problems I noticed:

[...]
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index f5fdaae..abc6ddd 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1136,7 +1136,9 @@ static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
>      AVIOContext *pb = s->pb;
>      AVCodecContext *codec = s->streams[pkt->stream_index]->codec;
>      int keyframe = !!(pkt->flags & AV_PKT_FLAG_KEY);
> -    int duration = pkt->duration;
> +    /* For backward compatibility, prefer convergence_duration. */
> +    int duration = pkt->convergence_duration > 0 ?
> +                   pkt->convergence_duration : pkt->duration;

  I think this should stay unchanged and instead...

>      int ret;
>      int64_t ts = mkv->tracks[pkt->stream_index].write_dts ? pkt->dts : pkt->pts;
>  
> @@ -1166,7 +1168,7 @@ static int mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
>          duration = mkv_write_srt_blocks(s, pb, pkt);
>      } else {
>          ebml_master blockgroup = start_ebml_master(pb, MATROSKA_ID_BLOCKGROUP, mkv_blockgroup_size(pkt->size));
> -        duration = pkt->convergence_duration;
> +        duration = pkt->duration;

  ...this should have the conditional as in the first hunk.

>          mkv_write_block(s, pb, MATROSKA_ID_BLOCK, pkt, 
>          put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
>          end_ebml_master(pb, blockgroup);
> diff --git a/libavformat/srtenc.c b/libavformat/srtenc.c
> index 171c45b..7c0119b 100644
> --- a/libavformat/srtenc.c
> +++ b/libavformat/srtenc.c
> @@ -64,7 +64,9 @@ static int srt_write_packet(AVFormatContext *avf, AVPacket *pkt)
>          int len;
>  
>          if (d <= 0)
> -            d = pkt->convergence_duration;
> +            /* For backward compatibility, prefer convergence_duration. */
> +            d = pkt->convergence_duration > 0 ?
> +                pkt->convergence_duration : pkt->duration;

  This should be wrong or at least redundant.

  AFAICT at that point d was already pkt->duration.

[...]

  Alexander