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

Philip Langdale philipl at overt.org
Wed Aug 29 05:55:52 CEST 2012


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.

Signed-off-by: Philip Langdale <philipl at overt.org>
---
 libavformat/matroskadec.c |   23 +++++++++++++++++++++--
 libavformat/matroskaenc.c |    5 ++++-
 libavformat/srtenc.c      |    1 +
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 1d0dbb3..99d3bb3 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 <= UINT_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 3977931..79ce9f5 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 6b8278f..0b094b0 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");
-- 
1.7.9.5



More information about the ffmpeg-devel mailing list