1366999 - Apply an accurate seek on a MP4 file might seek to somewhere beyond the seek target.

Status: RESOLVED WONTFIX

(Core :: Audio/Video: Playback, defect)

Description

[Tzuhao Kuo [:kaku]]

Attachment: wrong_seek_result.log

Steps to reproduce:
1. Go to https://people-mozilla.org/~tkuo/test/resources/wrong_container_info.mp4
2. Seek to any time between 6 to 10 seconds.


Actual results:
The seeked frame is at 10.01 seconds.


Expected results:
We should seek the video to a frame with its range contains the seek target.


Related logs: (see the attachment for the full log)
[MediaPlayback #2]: D/MediaDecoder Decoder=0x11aab1800 state=DECODING Changed state to SEEKING (to 6737000)
[MediaPlayback #2]: D/MediaDecoder Decoder=0x11aab1800 state=DECODING change state to: SEEKING
[MediaPlayback #2]: D/MediaDecoder Decoder=0x11aab1800 state=DECODING Exiting DECODING, decoded for 5.174s
[MediaPlayback #2]: D/MediaDecoder Decoder=0x11aab1800 StopPlayback()
[MediaPlayback #2]: D/MediaDecoder Decoder=0x11aab1800 Changed mNextFrameStatus to NEXT_FRAME_UNAVAILABLE_SEEKING
[MediaPlayback #2]: D/MediaDecoder Decoder=0x11aab1800 MediaDecoderStateMachine::Reset
[MediaPlayback #2]: D/MediaDecoder Decoder=0x11aab1800 Stop MediaSink
[MediaPlayback #2]: D/MediaDecoder VideoSink=0x11caaaab0 [Stop]
[MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x11cafd000)::ResetDecode: 
[MediaPlayback #2]: D/MediaFormatReader MediaFormatReader(0x11cafd000)::Reset: Reset(Video) BEGIN
[MediaPlayback #2]: D/MediaFormatReader MediaFormatReader(0x11cafd000)::Reset: Reset(Video) END
[MediaPlayback #2]: D/MediaFormatReader MediaFormatReader(0x11cafd000)::Reset: Reset(Audio) BEGIN
[MediaPlayback #2]: D/MediaFormatReader MediaFormatReader(0x11cafd000)::Reset: Reset(Audio) END
[MediaPlayback #2]: D/MediaFormatReader MediaFormatReader(0x11cafd000)::Seek: aTarget=(6737000)
[MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x11cafd000)::ScheduleUpdate: SchedulingUpdate(Video)
[MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x11cafd000)::ScheduleUpdate: SchedulingUpdate(Audio)
[MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x11cafd000)::DoVideoSeek: Seeking video to 6737000
[MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x11cafd000)::Update: Processing update for Video
[MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x11cafd000)::Update: Seeking hasn't completed, nothing more to do
[MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x11cafd000)::Update: Processing update for Audio
[MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x11cafd000)::Update: Update(Audio) ni=0 no=0 in:0 out:0 qs=0 decoding:0 flushing:0 desc:apple CoreMedia decoder pending:0 waiting:0 sid:4294967295
[MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x11cafd000)::Update: No need for additional input (pending:0)
[MediaPlayback #3]: V/MediaFormatReader MediaFormatReader(0x11cafd000)::OnVideoSeekCompleted: Video seeked to 10010000


The root cause is at this period of code.
http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/dom/media/fmp4/MP4Demuxer.cpp#415-433

In MP4TrackDemuxer::Seek(), we ask the SampleIterator::Seek() to seek to our target, and it locates to the keyframe previous to our target. Then, we call MP4TrackDemuxer::GetNextSample() to get the current frame out, however, in MP4TrackDemuxer::GetNextSample(), we double check the frame is a keyframe or not with a criterion which is different to what SampleIterator::Seek() uses. 

SampleIterator::Seek() checks a frame is a key frame or not by checking the `mSync` member. 
http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/media/libstagefright/binding/Index.cpp#274-277

On the other hand, MP4TrackDemuxer::GetNextSample() uses H264::GetFrameType() to check if the frame is an IDR frame or not.
http://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/media/libstagefright/binding/H264.cpp#949-952

The "sync" frame might not be an IDR frame, and we keep getting next samples until we meet an IDR frame, which might have already passed the seek target time.

Comment 1

[Tzuhao Kuo [:kaku]]

I think we can have the SampleIterator::Seek() respects the IDR information as well, however, it might hurt performance. 

:jay, may I have your thoughts?

Comment 2

[Jean-Yves Avenard [:jya]]

The file is badly mixed and is invalid. 

The only way to verify that the frame actually contains an IDR would be to read the data from the resource. 

As such, to seek to the end of the file, the only way to be sure would be to read the entire file first (as you can't trust the sample table, you must read from the start)

You'll find that other browsers badly seek in those type of files, the the image appearing frozen for a while.

Comment 3

[Tzuhao Kuo [:kaku]]

I agree that checking IDR hurts performance, but the problem is that the current behavior is just not right, isn't it? 
I mean that it is expected to seek to where we aimed to, instead of somewhere beyond the target. 

If the performance downside of checking IDR is not acceptable, is it okay to just trust the sample table (the sync information)? In this way, we might have few broken frames after seeking.

:jw, WDYT?

Comment 4

[Jean-Yves Avenard [:jya]]

We have no other choice but checking the sample table. 

Performance impact is a massive understatement. 
You would potentially read 4GB of data just to make sure you seek to the exact frame you need. It would take hours. 

It's a case of crap in crap out. 

The sample table is an index of the content with information on where to go to find IDR. If that table is incorrect and keyframes are incorrectly marked. Then seeking will be broken. 

Even if we didn't skip until the next keyframe, the decoder upon being fed a non keyframe would either error (like on mac) or drop it. 

You'll find that all video players behave the same. When you seek it freezes the video for a little while (as audio seeking did work) 

Mind you, with MSE as we do demux all the frames first, we are checking every single NAL and properly tag them as keyframe. So seeking there will work. But that's only possible because the whole file is in RAM.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

I prefer to just trust the sample table.

With a bad sample table, the user just suffers a few seconds after seeking. Doing IDR on a bad table, the user might suffer minutes from long seeking latency.

Comment 6

[Tzuhao Kuo [:kaku]]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> Even if we didn't skip until the next keyframe, the decoder upon being fed a
> non keyframe would either error (like on mac) or drop it. 
Would it be better to serach for the IDR frame backward here?
http://searchfox.org/mozilla-central/rev/a14524a72de6f4ff738a5e784970f0730cea03d8/dom/media/fmp4/MP4Demuxer.cpp#419-433

So that we're not able to seek to somewhere beyond the target.

Comment 7

[Jean-Yves Avenard [:jya]]

But can you statically prove that a search backward will not significantly cause greater time either.
The data in a MP4 is typically interleaved. Having to read the actual content just to search for a real IDR rather than read an index could still cause heave I/O.

Also note that in bug 1359058 it wouldn't have helped at all, in fact it would have been much worse. Those files had no IDR at all, yet all were marked as IDR.

So if you had seeked toward the end, you would have to read the entire file first.

Comment 8

[Tzuhao Kuo [:kaku]]

Reopen, since the phenomenon is reported by community.