1244639 - Seeking to 0 with MP3 may not return the first frame or seeking to the wrong frame

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

This appeared when I reverted https://hg.mozilla.org/integration/mozilla-inbound/rev/4b812ca9

It caused permafail in some of the seek test.

https://treeherder.mozilla.org/logviewer.html#?job_id=20807683&repo=mozilla-inbound

This commit hid the problem in the MP3 demuxer because it always returned min(0, audio->mTime).

The first mp3 frame decoded has a time of 0; as it should.

But when you seek to 0, the first frame returned after that as a time of 0.026122.

It should be 0. Otherwise currentTime will not be set to 0

Comment 1

[Jean-Yves Avenard [:jya]]

The description above is for dom/media/test/test_seek-9.html

For dom/media/test/test_seek-12.html, we seek to 1.671500s and the first frame returned is at 1.671808s which is past the seek time ; new current time should always be <= seek time.
Now this appears to be intermittent and the first frame returned after a seek varies.

Comment 2

[Jean-Yves Avenard [:jya]]

In bug 1243608, I added a work around this bug. it will have to be removed once this bug is fixed

Comment 3

[Eugen Sawin [:esawin]]

I've set up a test similar to test_seek-9.html [1] and it doesn't reproduce on Linux with both code versions.

On Android, we don't load meta data on page load, so this test would fail without autoplay enabled, but that's not going through the decoder/demuxer path at all.
Once the media was played, the seek position seems clamped properly.

I could see how the demuxer would misbehave should we pass the raw seek value to it (we don't test for < 0), but that doesn't seem to be the case here. Let me fix this anyways.

[1] http://me73.com/audio (file 5 + seek button)

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1244639-1.1-Clamp-negative-seek-position-to-zero.patch

Treat negative seek positions as zero but retain original values.

Comment 5

[Jean-Yves Avenard [:jya]]

There is no negative seek issue here.
You can never get the MP3Demuxer with a negative seek. The HTMLMediaElement will have sanitize the value well before it gets to the MP3Demuxer.

The problem is that is with the file dom/media/test/owl.mp3 when you seek to 0, the next frame it demux will have a time in the future (that is > 0)

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8715000 [details] [diff] [review]
0001-Bug-1244639-1.1-Clamp-negative-seek-position-to-zero.patch

Review of attachment 8715000 [details] [diff] [review]:
-----------------------------------------------------------------

You won't be called with negative value. this is not the problem at hand

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Don't assume MP3 decoding always starts at 0.

This remove the MP3 workaround for the mac decoder that was added in bug 1194080.

Yet another case of a workaround biting us months later.

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: P2. Don't clamp audio time to seek time if there's no video track.

This remove the need for working around the MP3 decoder returning decoded frames in the future.

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: P2. Don't clamp audio time to seek time if there's no video track.

This remove the need for working around the MP3 decoder returning decoded frames in the future.

Looks like I was a tad too enthousiastic in removing some code I had deemded unecessary. audio may be null when seeking at the end of the file.

Comment 10

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8715094 [details] [diff] [review]
Don't assume MP3 decoding always starts at 0.

Review of attachment 8715094 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaFormatReader.cpp
@@ +1598,5 @@
>    } else if (HasVideo()) {
>      intervals = Move(videoti);
>    }
>  
> +  if (!startTime || !intervals.Length() ||

Can you make shifting by 0, or shifting an empty TimeIntervals a noop? Then you don't need the first two conditions here.

Is it the case that (intervals.GetStart() == media::TimeUnit::FromMicroseconds(0)) can be true when the start time is non-zero? Does it make sense to override the start time in this case? I'm just wondering if we can get away without the branch here at all.

Comment 12

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #11)
> Comment on attachment 8715094 [details] [diff] [review]
> Don't assume MP3 decoding always starts at 0.
> 
> Review of attachment 8715094 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaFormatReader.cpp
> @@ +1598,5 @@
> >    } else if (HasVideo()) {
> >      intervals = Move(videoti);
> >    }
> >  
> > +  if (!startTime || !intervals.Length() ||
> 
> Can you make shifting by 0, or shifting an empty TimeIntervals a noop? Then
> you don't need the first two conditions here.
> 
> Is it the case that (intervals.GetStart() ==
> media::TimeUnit::FromMicroseconds(0)) can be true when the start time is
> non-zero? Does it make sense to override the start time in this case? I'm
> just wondering if we can get away without the branch here at all.

I can remove the test for startTime , because this shifting will be a no-op.
Not sure I follow the second.
If the start time is non 0, then it will never be equal to media::TimeUnit::FromMicroseconds(0).

If the start time is non 0, then in theory the buffered range should always start at "start time" too. except that it doesn't because some demuxers approximate their buffered range based on the bitrate and always assume start time is 0.

The problem here is that we have a discrepency between what the demuxer believe the buffered range is, and the decoder may return something else.
It would be the same with Vorbis, the first sample never returns decoded data, we need two.

Comment 13

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #11)
> Is it the case that (intervals.GetStart() ==
> media::TimeUnit::FromMicroseconds(0)) can be true when the start time is
> non-zero? Does it make sense to override the start time in this case? I'm
> just wondering if we can get away without the branch here at all.

Upon re-reading...

Yes, start time can be non zero (it happens when playing owl.mp3 on a mac: it's 0.0533) yet the MP3Demuxer returns a buffered range starting at 0. this is due to the discrepency between the demuxer and the decoder I referred to above.
And in fact, this case is the only one we care about.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5adde7c353a
https://hg.mozilla.org/integration/mozilla-inbound/rev/772b1669c5b3

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f5adde7c353a
https://hg.mozilla.org/mozilla-central/rev/772b1669c5b3