Closed Bug 1244639 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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
Flags: needinfo?(esawin)
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.
Summary: Seeking to 0 with MP3 may not return the first frame. → Seeking to 0 with MP3 may not return the first frame or seeking to the wrong frame
Keywords: regression
Priority: -- → P1
In bug 1243608, I added a work around this bug. it will have to be removed once this bug is fixed
Blocks: 1243608
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)
Flags: needinfo?(esawin)
Treat negative seek positions as zero but retain original values.
Attachment #8715000 - Flags: review?(jyavenard)
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 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
Attachment #8715000 - Flags: review?(jyavenard) → review-
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.
Attachment #8715094 - Flags: review?(cpearce)
This remove the need for working around the MP3 decoder returning decoded frames in the future.
Attachment #8715095 - Flags: review?(cpearce)
Assignee: nobody → jyavenard
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.
Attachment #8715120 - Flags: review?(cpearce)
Attachment #8715095 - Attachment is obsolete: true
Attachment #8715095 - Flags: review?(cpearce)
Priority: P1 → P2
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.
Attachment #8715094 - Flags: review?(cpearce) → review+
Attachment #8715120 - Flags: review?(cpearce) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/f5adde7c353a
https://hg.mozilla.org/mozilla-central/rev/772b1669c5b3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.