diff mbox

[FFmpeg-devel] libavformat/mov.c: use calculated dts offset when seeking in streams

Message ID 20171029111139.8293-1-pegro@friiks.de
State Accepted
Commit 59ad504696958fbd9db7b478b4b7e0a2b436b7f2
Headers show

Commit Message

Peter Große Oct. 29, 2017, 11:11 a.m. UTC 
From: Jonas Licht <jonas.licht@fem.tu-ilmenau.de>

Subtract the calculated dts offset from the requested timestamp before
seeking. This fixes an error "Error while filtering: Operation not
permitted" observed with a short file which contains only one key frame
and starts with negative timestamps.

Then, av_index_search_timestamp() returns a valid negative timestamp,
but mov_seek_stream bails out with AVERROR_INVALIDDATA.

Fixes ticket #6139.

Signed-off-by: Jonas Licht <jonas.licht@fem.tu-ilmenau.de>
Signed-off-by: Peter Große <pegro@friiks.de>
---

Compared to the other seek results in this test, the values are not
that far off.

 libavformat/mov.c        |  6 ++++--
 tests/ref/seek/extra-mp4 | 16 ++++++++--------
 2 files changed, 12 insertions(+), 10 deletions(-)

Comments

Michael Niedermayer Oct. 30, 2017, 12:03 a.m. UTC | #1 
On Sun, Oct 29, 2017 at 12:11:39PM +0100, Peter Große wrote:
> From: Jonas Licht <jonas.licht@fem.tu-ilmenau.de>
> 
> Subtract the calculated dts offset from the requested timestamp before
> seeking. This fixes an error "Error while filtering: Operation not
> permitted" observed with a short file which contains only one key frame
> and starts with negative timestamps.
> 
> Then, av_index_search_timestamp() returns a valid negative timestamp,
> but mov_seek_stream bails out with AVERROR_INVALIDDATA.
> 
> Fixes ticket #6139.
> 
> Signed-off-by: Jonas Licht <jonas.licht@fem.tu-ilmenau.de>
> Signed-off-by: Peter Große <pegro@friiks.de>
> ---
> 
> Compared to the other seek results in this test, the values are not
> that far off.
> 
>  libavformat/mov.c        |  6 ++++--
>  tests/ref/seek/extra-mp4 | 16 ++++++++--------
>  2 files changed, 12 insertions(+), 10 deletions(-)

will apply
thanks

[...]
Sasi Inguva March 5, 2018, 11:53 p.m. UTC | #2 
This patch seems to be doing the wrong thing and breaking seek tests for us.

As far as I understand , seeking for most containers is based on "decoding
timestamp". Unless  AV_SEEK_TO_PTS flag is specified in container, which is
not for most containers and MOV.  So if PTS and DTS are like such,
Pts  Dts
 0  -2   : frame0
 1  -1   : frame1
 2   0   : frame2
 3   1   : frame3
...
Seeking  to "0" timestamp without any flags, I should expect frame2 . But
instead this patch will give me frame0 . The patch's intention seems to be
seeking based on PTS (subtracting by the sc->time_offset essentially is a
mapping from PTS to DTS) .

To fix your specific issue https://trac.ffmpeg.org/ticket/6139
<https://www.google.com/url?q=https://trac.ffmpeg.org/ticket/6139&sa=D&usg=AFQjCNHI9l7YGXQI-LzJcyi5EWkH1PnaDw>
,
you can use AV_SEEK_BACKWARD flag while seeking in seek_to_start
http://git.videolan.org/?p=ffmpeg.git;a=blob;f=fftools/ffmpeg.c;h=ee7258fcd1f65b6670f19686b51d5b41ee72f7ac;hb=HEAD#l4165


On Sun, Oct 29, 2017 at 4:11 AM, Peter Große <pegro@friiks.de> wrote:

> From: Jonas Licht <jonas.licht@fem.tu-ilmenau.de>
>
> Subtract the calculated dts offset from the requested timestamp before
> seeking. This fixes an error "Error while filtering: Operation not
> permitted" observed with a short file which contains only one key frame
> and starts with negative timestamps.
>
> Then, av_index_search_timestamp() returns a valid negative timestamp,
> but mov_seek_stream bails out with AVERROR_INVALIDDATA.
>
> Fixes ticket #6139.
>
> Signed-off-by: Jonas Licht <jonas.licht@fem.tu-ilmenau.de>
> Signed-off-by: Peter Große <pegro@friiks.de>
> ---
>
> Compared to the other seek results in this test, the values are not
> that far off.
>
>  libavformat/mov.c        |  6 ++++--
>  tests/ref/seek/extra-mp4 | 16 ++++++++--------
>  2 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 2ee67561e4..60a0f4ccf4 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -6882,10 +6882,12 @@ static int mov_seek_fragment(AVFormatContext *s,
> AVStream *st, int64_t timestamp
>  static int mov_seek_stream(AVFormatContext *s, AVStream *st, int64_t
> timestamp, int flags)
>  {
>      MOVStreamContext *sc = st->priv_data;
> -    int sample, time_sample;
> +    int sample, time_sample, ret;
>      unsigned int i;
>
> -    int ret = mov_seek_fragment(s, st, timestamp);
> +    timestamp -= sc->time_offset;
> +
> +    ret = mov_seek_fragment(s, st, timestamp);
>      if (ret < 0)
>          return ret;
>
> diff --git a/tests/ref/seek/extra-mp4 b/tests/ref/seek/extra-mp4
> index c25544c095..c17ce4003c 100644
> --- a/tests/ref/seek/extra-mp4
> +++ b/tests/ref/seek/extra-mp4
> @@ -28,10 +28,10 @@ ret: 0         st: 0 flags:0 dts: 50.633333 pts:
> 50.733333 pos:5926157 size:  13
>  ret: 0         st: 0 flags:0 dts: 50.666667 pts: 50.666667 pos:5927464
> size:   150
>  ret: 0         st: 0 flags:0 dts: 50.700000 pts: 50.700000 pos:5927614
> size:   176
>  ret: 0         st:-1 flags:1  ts: 153.470835
> -ret: 0         st: 0 flags:1 dts: 153.466667 pts: 153.500000 pos:15867700
> size: 96169
> -ret: 0         st: 0 flags:0 dts: 153.500000 pts: 153.533333 pos:15963869
> size:   785
> -ret: 0         st: 0 flags:0 dts: 153.533333 pts: 153.633333 pos:15964654
> size:  3135
> -ret: 0         st: 0 flags:0 dts: 153.566667 pts: 153.566667 pos:15967789
> size:   859
> +ret: 0         st: 0 flags:1 dts: 151.966667 pts: 152.000000 pos:15705355
> size:146924
> +ret: 0         st: 0 flags:0 dts: 152.000000 pts: 152.100000 pos:15852279
> size:  1355
> +ret: 0         st: 0 flags:0 dts: 152.033333 pts: 152.033333 pos:15853634
> size:   211
> +ret: 0         st: 0 flags:0 dts: 152.066667 pts: 152.066667 pos:15853845
> size:   217
>  ret: 0         st: 0 flags:0  ts: 76.365000
>  ret: 0         st: 0 flags:1 dts: 77.833333 pts: 77.866667 pos:8659657
> size: 41182
>  ret: 0         st: 0 flags:0 dts: 77.866667 pts: 77.966667 pos:8700839
> size:  4197
> @@ -83,10 +83,10 @@ ret: 0         st: 0 flags:0 dts: 101.333333 pts:
> 101.433333 pos:11049548 size:
>  ret: 0         st: 0 flags:0 dts: 101.366667 pts: 101.366667 pos:11053072
> size:   562
>  ret: 0         st: 0 flags:0 dts: 101.400000 pts: 101.400000 pos:11053634
> size:   599
>  ret: 0         st:-1 flags:0  ts: 25.306672
> -ret: 0         st: 0 flags:1 dts: 27.400000 pts: 27.433333 pos:2674605
> size:127383
> -ret: 0         st: 0 flags:0 dts: 27.433333 pts: 27.466667 pos:2801988
> size:    68
> -ret: 0         st: 0 flags:0 dts: 27.466667 pts: 27.500000 pos:2802268
> size:  1754
> -ret: 0         st: 0 flags:0 dts: 27.500000 pts: 27.533333 pos:2804022
> size:  4071
> +ret: 0         st: 0 flags:1 dts: 25.300000 pts: 25.333333 pos:2607246
> size: 40273
> +ret: 0         st: 0 flags:0 dts: 25.333333 pts: 25.433333 pos:2647519
> size:  2959
> +ret: 0         st: 0 flags:0 dts: 25.366667 pts: 25.366667 pos:2650478
> size:   197
> +ret: 0         st: 0 flags:0 dts: 25.400000 pts: 25.400000 pos:2650675
> size:   230
>  ret: 0         st:-1 flags:1  ts: 128.200839
>  ret: 0         st: 0 flags:1 dts: 127.833333 pts: 127.866667 pos:13514072
> size: 67382
>  ret: 0         st: 0 flags:0 dts: 127.866667 pts: 127.966667 pos:13581454
> size:  2936
> --
> 2.13.6
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer March 7, 2018, 11:27 a.m. UTC | #3 
On Mon, Mar 05, 2018 at 03:53:04PM -0800, Sasi Inguva wrote:
> This patch seems to be doing the wrong thing and breaking seek tests for us.
> 
> As far as I understand , seeking for most containers is based on "decoding
> timestamp". Unless  AV_SEEK_TO_PTS flag is specified in container, which is
> not for most containers and MOV.  So if PTS and DTS are like such,

I think most seeking in libavformat uses whatever was convenient for the 
implementation of each demuxer. Some are much more carefully implemented
than others. Iam not so sure its consistently DTS when AVFMT_SEEK_TO_PTS
is not set.


> Pts  Dts
>  0  -2   : frame0
>  1  -1   : frame1
>  2   0   : frame2
>  3   1   : frame3
> ...
> Seeking  to "0" timestamp without any flags, I should expect frame2 . But
> instead this patch will give me frame0 . The patch's intention seems to be
> seeking based on PTS (subtracting by the sc->time_offset essentially is a
> mapping from PTS to DTS) .

IMO, seeking per PTS is more usefull to end users, the user gets what he asked
for, a frame that he can display at that requested time.
The ultimate goal should be to have proper frame exact seeking or as close to
it as practically implementable.

DTS is sometimes easier to implement on our side. 
But with seeking per DTS it is a heuristic gamble on how to get a frame to
display at a specific timestamp. Its possible with seek per dts to never
get a frame displayable at the time requested.

For formats with a full index like mov/mp4 seeking per pts should be IMHO be
preferred over dts.

There are more corner cases that require mentioning, like for example that there
can be a B frame after a I frame with a pts prior to the I frame.
as in

coded order:
Pts Dts 
1  -1   I frame
0   0   B frame
3   1   P frame
2   2   B frame
It is desirable and valid to seek to the I frame at pts=1 for a seek targeting 
the B frame  at pts=0 if and only if this B frame can be displayed and 
does not depend on a frame of the prior GOP (streams generally have flags
in their headers for this specific case to be detectable)

Furthermore, if the goal is to seek to pts=5 and there is a keyframe at pts=5
and one at pts=4. And there is another stream that can only be decoded at pts=5
if demuxing starts at pts=4 then the demuxer can seek to pts=4 instead of 5.
This is especially the case for containers with subtitles where their
display may require positioning at a earlier place and then potentially
discarding packets in streams that are unneeded.

also avformat_seek_file() allows much more flexibility for the user to specify
where and how to seek. And where the av_seek_frame() API does not specify
what timestamp is used. avformat_seek_file() does specify that
"can be presented successfully" and can thus not be just DTS


[...]
Sasi Inguva March 9, 2018, 9:24 p.m. UTC | #4 
Seek on files with empty edit lists is failing with this patch. For a file
with empty edit of 1s. (media time: -1 , duration : 1 ) , both DTS and PTS
start at 1s.  Also sc->time_offset is computed as   -1s.  Hence when we
seek to timestamp t, it will actually seek to timestamp t+1 , which is
wrong because there is no offset between DTS and PTS in this case .

The offset by which to shift timestamp, can be computed more accurately.
I'll send a patch to fix this.


On Wed, Mar 7, 2018 at 3:27 AM, Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Mon, Mar 05, 2018 at 03:53:04PM -0800, Sasi Inguva wrote:
> > This patch seems to be doing the wrong thing and breaking seek tests for
> us.
> >
> > As far as I understand , seeking for most containers is based on
> "decoding
> > timestamp". Unless  AV_SEEK_TO_PTS flag is specified in container, which
> is
> > not for most containers and MOV.  So if PTS and DTS are like such,
>
> I think most seeking in libavformat uses whatever was convenient for the
> implementation of each demuxer. Some are much more carefully implemented
> than others. Iam not so sure its consistently DTS when AVFMT_SEEK_TO_PTS
> is not set.
>
>
> > Pts  Dts
> >  0  -2   : frame0
> >  1  -1   : frame1
> >  2   0   : frame2
> >  3   1   : frame3
> > ...
> > Seeking  to "0" timestamp without any flags, I should expect frame2 . But
> > instead this patch will give me frame0 . The patch's intention seems to
> be
> > seeking based on PTS (subtracting by the sc->time_offset essentially is a
> > mapping from PTS to DTS) .
>
> IMO, seeking per PTS is more usefull to end users, the user gets what he
> asked
> for, a frame that he can display at that requested time.
> The ultimate goal should be to have proper frame exact seeking or as close
> to
> it as practically implementable.
>
> DTS is sometimes easier to implement on our side.
> But with seeking per DTS it is a heuristic gamble on how to get a frame to
> display at a specific timestamp. Its possible with seek per dts to never
> get a frame displayable at the time requested.
>
> For formats with a full index like mov/mp4 seeking per pts should be IMHO
> be
> preferred over dts.
>
> I agree. But the index in MOV (st->index_entries) is  based on DTS . Exact
seek based on PTS will be hard to implement.  Just subtracting a fixed
offset does not suffice to map PTS to DTS ( especially when there are B
frames) .  To seek to the correct frame , apart from doing
av_index_search_timestamp , we would have to go back and formulate the PTS
using CTTS and search in the neighborhood for correct PTS.

There are more corner cases that require mentioning, like for example that
> there
> can be a B frame after a I frame with a pts prior to the I frame.
> as in
>
> coded order:
> Pts Dts
> 1  -1   I frame
> 0   0   B frame
> 3   1   P frame
> 2   2   B frame
> It is desirable and valid to seek to the I frame at pts=1 for a seek
> targeting
> the B frame  at pts=0 if and only if this B frame can be displayed and
> does not depend on a frame of the prior GOP (streams generally have flags
> in their headers for this specific case to be detectable)
>
> Furthermore, if the goal is to seek to pts=5 and there is a keyframe at
> pts=5
> and one at pts=4. And there is another stream that can only be decoded at
> pts=5
> if demuxing starts at pts=4 then the demuxer can seek to pts=4 instead of
> 5.
> This is especially the case for containers with subtitles where their
> display may require positioning at a earlier place and then potentially
> discarding packets in streams that are unneeded.
>
> also avformat_seek_file() allows much more flexibility for the user to
> specify
> where and how to seek. And where the av_seek_frame() API does not specify
> what timestamp is used. avformat_seek_file() does specify that
> "can be presented successfully" and can thus not be just DTS
>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> It is what and why we do it that matters, not just one of them.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
diff mbox

Patch

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 2ee67561e4..60a0f4ccf4 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -6882,10 +6882,12 @@  static int mov_seek_fragment(AVFormatContext *s, AVStream *st, int64_t timestamp
 static int mov_seek_stream(AVFormatContext *s, AVStream *st, int64_t timestamp, int flags)
 {
     MOVStreamContext *sc = st->priv_data;
-    int sample, time_sample;
+    int sample, time_sample, ret;
     unsigned int i;
 
-    int ret = mov_seek_fragment(s, st, timestamp);
+    timestamp -= sc->time_offset;
+
+    ret = mov_seek_fragment(s, st, timestamp);
     if (ret < 0)
         return ret;
 
diff --git a/tests/ref/seek/extra-mp4 b/tests/ref/seek/extra-mp4
index c25544c095..c17ce4003c 100644
--- a/tests/ref/seek/extra-mp4
+++ b/tests/ref/seek/extra-mp4
@@ -28,10 +28,10 @@  ret: 0         st: 0 flags:0 dts: 50.633333 pts: 50.733333 pos:5926157 size:  13
 ret: 0         st: 0 flags:0 dts: 50.666667 pts: 50.666667 pos:5927464 size:   150
 ret: 0         st: 0 flags:0 dts: 50.700000 pts: 50.700000 pos:5927614 size:   176
 ret: 0         st:-1 flags:1  ts: 153.470835
-ret: 0         st: 0 flags:1 dts: 153.466667 pts: 153.500000 pos:15867700 size: 96169
-ret: 0         st: 0 flags:0 dts: 153.500000 pts: 153.533333 pos:15963869 size:   785
-ret: 0         st: 0 flags:0 dts: 153.533333 pts: 153.633333 pos:15964654 size:  3135
-ret: 0         st: 0 flags:0 dts: 153.566667 pts: 153.566667 pos:15967789 size:   859
+ret: 0         st: 0 flags:1 dts: 151.966667 pts: 152.000000 pos:15705355 size:146924
+ret: 0         st: 0 flags:0 dts: 152.000000 pts: 152.100000 pos:15852279 size:  1355
+ret: 0         st: 0 flags:0 dts: 152.033333 pts: 152.033333 pos:15853634 size:   211
+ret: 0         st: 0 flags:0 dts: 152.066667 pts: 152.066667 pos:15853845 size:   217
 ret: 0         st: 0 flags:0  ts: 76.365000
 ret: 0         st: 0 flags:1 dts: 77.833333 pts: 77.866667 pos:8659657 size: 41182
 ret: 0         st: 0 flags:0 dts: 77.866667 pts: 77.966667 pos:8700839 size:  4197
@@ -83,10 +83,10 @@  ret: 0         st: 0 flags:0 dts: 101.333333 pts: 101.433333 pos:11049548 size:
 ret: 0         st: 0 flags:0 dts: 101.366667 pts: 101.366667 pos:11053072 size:   562
 ret: 0         st: 0 flags:0 dts: 101.400000 pts: 101.400000 pos:11053634 size:   599
 ret: 0         st:-1 flags:0  ts: 25.306672
-ret: 0         st: 0 flags:1 dts: 27.400000 pts: 27.433333 pos:2674605 size:127383
-ret: 0         st: 0 flags:0 dts: 27.433333 pts: 27.466667 pos:2801988 size:    68
-ret: 0         st: 0 flags:0 dts: 27.466667 pts: 27.500000 pos:2802268 size:  1754
-ret: 0         st: 0 flags:0 dts: 27.500000 pts: 27.533333 pos:2804022 size:  4071
+ret: 0         st: 0 flags:1 dts: 25.300000 pts: 25.333333 pos:2607246 size: 40273
+ret: 0         st: 0 flags:0 dts: 25.333333 pts: 25.433333 pos:2647519 size:  2959
+ret: 0         st: 0 flags:0 dts: 25.366667 pts: 25.366667 pos:2650478 size:   197
+ret: 0         st: 0 flags:0 dts: 25.400000 pts: 25.400000 pos:2650675 size:   230
 ret: 0         st:-1 flags:1  ts: 128.200839
 ret: 0         st: 0 flags:1 dts: 127.833333 pts: 127.866667 pos:13514072 size: 67382
 ret: 0         st: 0 flags:0 dts: 127.866667 pts: 127.966667 pos:13581454 size:  2936