matroskadec, matroskadec, srtenc: Read/Write duration for subtitles.

After much discussion and back-and-forth, we reached the conclusion
that matroska uses convergence_duration for subtitle duration because
a 32bit value isn't large enough to store the duration if sub-micro-second
timebases are used. Matroska may not be the only one that supports these
timebases, but it's certainly the only one that ffmpeg attempts to support
in this way.

The long term solution that we seemed to reach was that if we encounter
a matroska file with a sub-micro-second timebase, we should internally
scale it up to at least micro-second, and then duration can be used
normally. This suggests that on the encode side, we should not allow
generation of files with sub-micro-second timebases, but that's a separate
issue.

That being a non-trivial change, and the subtitle interoperability breakage
being very real, I'm re-submitting this small change for consideration.

In this diff, we make sure that duration is populated by the matroska
demuxer, and that convergence_duration is respected in matroskaenc and
srtenc, but that duration is used otherwise. This ends up being a strict
improvement - pipelines that use convergence duration are unchanged, and
ones that are currently broken due to the duration mismatch will start
working - except for the ones with the extreme timebases, but those were
already broken.

Signed-off-by: Philip Langdale <philipl@overt.org>

3 files changed, 26 insertions, 3 deletions

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 1d0dbb3296..c4e0438c1d 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -2105,10 +2105,29 @@ static int matroska_parse_block(MatroskaDemuxContext *matroska, uint8_t *data,
                 else
                     pkt->pts = timecode;
                 pkt->pos = pos;
-                if (st->codec->codec_id == AV_CODEC_ID_SUBRIP)
+                if (st->codec->codec_id == AV_CODEC_ID_SUBRIP) {
+                    /*
+                     * For backward compatibility.
+                     * Historically, we have put subtitle duration
+                     * in convergence_duration, on the off chance
+                     * that the time_scale is less than 1us, which
+                     * could result in a 32bit overflow on the
+                     * normal duration field.
+                     */
                     pkt->convergence_duration = lace_duration;
-                else if (track->type != MATROSKA_TRACK_TYPE_SUBTITLE)
+                }
+
+                if (track->type != MATROSKA_TRACK_TYPE_SUBTITLE ||
+                    lace_duration <= INT_MAX) {
+                    /*
+                     * For non subtitle tracks, just store the duration
+                     * as normal.
+                     *
+                     * If it's a subtitle track and duration value does
+                     * not overflow a uint32, then also store it normally.
+                     */
                     pkt->duration = lace_duration;
+                }
 
                 if (st->codec->codec_id == AV_CODEC_ID_SSA)
                     matroska_fix_ass_packet(matroska, pkt, lace_duration);
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 397793119e..79ce9f5cae 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -1168,7 +1168,10 @@ 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;
+        /* For backward compatibility, prefer convergence_duration. */
+        if (pkt->convergence_duration > 0) {
+            duration = pkt->convergence_duration;
+        }
         mkv_write_block(s, pb, MATROSKA_ID_BLOCK, pkt, 0);
         put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
         end_ebml_master(pb, blockgroup);
diff --git a/libavformat/srtenc.c b/libavformat/srtenc.c
index 6b8278fa80..0b094b00bf 100644
--- a/libavformat/srtenc.c
+++ b/libavformat/srtenc.c
@@ -65,6 +65,7 @@ static int srt_write_packet(AVFormatContext *avf, AVPacket *pkt)
         int len;
 
         if (d <= 0)
+            /* For backward compatibility, fallback to convergence_duration. */
             d = pkt->convergence_duration;
         if (s == AV_NOPTS_VALUE || d <= 0) {
             av_log(avf, AV_LOG_ERROR, "Insufficient timestamps.\n");