Closed Bug 1314552 Opened 3 years ago Closed 3 years ago

MDSM::mMediaSeekableOnlyInBufferedRanges should be updated by metadata instead of MediaDecoder

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(2 files)

Assignee: nobody → jwwang
Blocks: 1286129
Priority: -- → P3
Attachment #8808454 - Flags: review?(jyavenard)
Attachment #8808455 - Flags: review?(jyavenard)
Comment on attachment 8808454 [details]
Bug 1314552. Part 1 - update mMediaSeekableOnlyInBufferedRanges in OnMetadataRead().

https://reviewboard.mozilla.org/r/91264/#review91098

As I mentioned last week, you are here assuming that the seekability is known when metadata is being read.
This is not always the case and is an incorrect assumption.

With chained ogg, the first ogg file will play normally, and this one is seekable.
Only once you've reached the end of the ogg file and go to a chained one, will the media becomes unseekable.

As such, you can't do what you are doing in this change. As you will lose the ability to transition from normal ogg to a chained ogg.
Comment on attachment 8808454 [details]
Bug 1314552. Part 1 - update mMediaSeekableOnlyInBufferedRanges in OnMetadataRead().

https://reviewboard.mozilla.org/r/91264/#review91098

http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/media/MediaDecoderStateMachine.h#819
Isn't that what mMediaSeekable is for?

MDSM::mMediaSeekableOnlyInBufferedRanges used to be mirrored from MediaDecoder::mMediaSeekableOnlyInBufferedRanges which is updated by the metadata. This bug basically changes nothing but removing canonical/mirror and update MDSM::mMediaSeekableOnlyInBufferedRanges directly from the metadata. The logic is basically the same.
Comment on attachment 8808454 [details]
Bug 1314552. Part 1 - update mMediaSeekableOnlyInBufferedRanges in OnMetadataRead().

https://reviewboard.mozilla.org/r/91264/#review91098

yes and no. mMediaSeekableOnlyInBufferedRanges is currently only used with WebM, and is used when playing webm with no seek cues (like those generated when recording a webrtc session). This one can be determined during AsyncReadMetadata as we can immediately know if the webm container has clues or not.

Then you have the asynchronous call, that can happen at anytime during the lifetime of the reader/demuxer.

https://dxr.mozilla.org/mozilla-central/source/dom/media/ogg/OggDemuxer.cpp#760

This uses the MediaDecoderReader::MediaNotSeekableProducer() mechanism
https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderReader.h#287
Comment on attachment 8808454 [details]
Bug 1314552. Part 1 - update mMediaSeekableOnlyInBufferedRanges in OnMetadataRead().

https://reviewboard.mozilla.org/r/91264/#review91108

Oh my bad...

This removal of the mirror of mMediaSeekableOnlyInBufferedRanges is okay.
However, it appears that MDSM::OnMediaNotSeekable() is no longer connected, that will break chained ogg
Comment on attachment 8808454 [details]
Bug 1314552. Part 1 - update mMediaSeekableOnlyInBufferedRanges in OnMetadataRead().

https://reviewboard.mozilla.org/r/91264/#review91110
Attachment #8808454 - Flags: review?(jyavenard) → review+
Comment on attachment 8808455 [details]
Bug 1314552. Part 2 - remove unused canonical.

https://reviewboard.mozilla.org/r/91266/#review91112
Attachment #8808455 - Flags: review?(jyavenard) → review+
Thanks!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b82e11c3127
Part 1 - update mMediaSeekableOnlyInBufferedRanges in OnMetadataRead(). r=jya
https://hg.mozilla.org/integration/autoland/rev/deb2dcbb0ec0
Part 2 - remove unused canonical. r=jya
https://hg.mozilla.org/mozilla-central/rev/8b82e11c3127
https://hg.mozilla.org/mozilla-central/rev/deb2dcbb0ec0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.