[FFmpeg-devel] [PATCH 1/2] lavf/subtitles: add ff_subtitles_queue_seek().

[Clément Bœsch]

On Tue, Nov 27, 2012 at 04:26:00PM +0100, Michael Niedermayer wrote:
> On Tue, Nov 27, 2012 at 04:21:55PM +0100, Clément Bœsch wrote:
> > On Mon, Nov 26, 2012 at 03:39:58AM +0100, Michael Niedermayer wrote:
> > [...]
> > > > > > +            if (pts >= min_ts && pts <= max_ts && ts_diff < min_ts_diff) {
> > > > > > +                min_ts_diff = ts_diff;
> > > > > > +                idx = i;
> > > > > > +            }
> > > > > 
> > > > > iam not sure this is correct, please correct me if i misunderstand
> > > > > something.
> > > > > consider there are subtitles, first one displays from 2 to 10 second
> > > > > second one displays from 5 to 18 seconds
> > > > > if you want to seek to second 8 you actually need to seek to 2 to
> > > > > get both displayed at your target
> > > > > 
> > > > 
> > > > I admit I didn't give much thoughts about it; as stated in the commit
> > > > description I took this code from lavf/assdec:read_seek2(), added in
> > > > ac066b83. The code is just adapted to work with FFDemuxSubtitlesQueue, and
> > > > it appeared to work fine with my simple tests so I didn't look much into
> > > > it.
> > > > 
> > > > Now if we want to honor the overlapping issue you're mentioning, we will
> > > > likely need to introduce a small threshold, such as limit to 5 or 10
> > > > seconds
> > > 
> > > > (imagine the case where a subtitle event from the beginning up to
> > > > 1 hour, like a banner, branding or something).
> > > 
> > > > BTW, I don't know what are
> > > > the consequences of a very incorrect seek with subtitles…
> > > 
> > > for a video with subtitles, you dont see the correct subtitles after
> > > seeking or in case of -ss with ffmpeg they are permanently missing
> > > in the transcoded file.
> > > That assumes here that you flush the renderer, seek and then feed it
> > > with the following subtitles.
> > > 
> > 
> > If we have a subtitle event starting at t=2sec, with duration=1hour,
> > followed by normal subtitles at a frequency of about 1000 subtitles per
> > hour, what happens if the seek at t=30min happens to actually seek at
> > t=2sec? What's gonna happen to the ~500 subtitles to keep up with the
> > current time? Remuxing them doesn't sound right, and same for making the
> > player display all of them as fast as possible.
> 
> can we discard such "past" subtitles somewhere conveniently, so that
> effectively only the 1h and the actually relevant subtitles after that
> are used ?
> (that could be done efficiently in the demuxer or less efficiently
>  afterwards)
> 
> > 
> > This is an extreme case, but similar situations are going to happen if we
> > start looking at solving these overlapping issues.
> > 
> > BTW, I'm a bit reluctant to changing this in this patch (I'm just
> > moving/copying the code to allow re-use it with all the subtitles format),
> > could we delay that feature, or you believe it's blocking?
> 
> delay is fine, but it has to be dealt with at some point, and
> preferably before too much code uses the function because it may
> become more painfull then if something needs to be changed for all
> users
> 
> 

I've queued this on top of the patchset to improve the seek. Would that be
good enough?

-- 
Clément B.
-------------- next part --------------
From beaf07e31f3ae8e8ade3da7b53cbe16fac9f64f7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux at gmail.com>
Date: Fri, 30 Nov 2012 07:00:59 +0100
Subject: [PATCH] lavf/subtitles: seek a little more backward when necessary.

If some previous subtitles are overlapping with the current time
we make sure they are raised so the renderer can display them too.
---
 libavformat/subtitles.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c
index 3661495..4e50f44 100644
--- a/libavformat/subtitles.c
+++ b/libavformat/subtitles.c
@@ -103,6 +103,7 @@ int ff_subtitles_queue_seek(FFDemuxSubtitlesQueue *q, AVFormatContext *s, int st
     } else {
         int i, idx = -1;
         int64_t min_ts_diff = INT64_MAX;
+        int64_t ts_selected;
         if (stream_index == -1) {
             AVRational time_base = s->streams[0]->time_base;
             ts = av_rescale_q(ts, AV_TIME_BASE_Q, time_base);
@@ -124,6 +125,16 @@ int ff_subtitles_queue_seek(FFDemuxSubtitlesQueue *q, AVFormatContext *s, int st
         }
         if (idx < 0)
             return AVERROR(ERANGE);
+        /* look back in the latest subtitles for overlapping subtitles */
+        ts_selected = q->subs[idx].pts;
+        for (i = idx - 1; i >= 0; i--) {
+            if (q->subs[i].duration <= 0)
+                continue;
+            if (q->subs[i].pts > ts_selected - q->subs[i].duration)
+                idx = i;
+            else
+                break;
+        }
         q->current_sub_idx = idx;
     }
     return 0;
-- 
1.8.0.1

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121130/d6275ba2/attachment.asc>