Closed
Bug 1161444
Opened 9 years ago
Closed 9 years ago
Change MP4Reader's IsMediaSeekable()
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: bwu, Assigned: bwu)
References
Details
Attachments
(1 file, 1 obsolete file)
I noticed this when checking the bug 1160241. IsMediaSeekable() should literally check the seekability of media content itself instead of checking the transport side.
Assignee | ||
Comment 1•9 years ago
|
||
ajones, Do you have concerns to check only mDemuxer->CanSeek()?
Flags: needinfo?(ajones)
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #2) > Wrong component... oops. Sorry! :-)
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Blake Wu [:bwu][:blakewu] from comment #1) > ajones, > Do you have concerns to check only mDemuxer->CanSeek()? To be more clear, I mean to only check mDemuxer->CanSeek()in IsMediaSeekable().
We don't properly support non-seekable MP4 anyway. What problem are you trying to solve?
Flags: needinfo?(ajones)
Assignee | ||
Comment 6•9 years ago
|
||
For B2G, we use MP4Reader to play local playback as well. However, on B2G current MP4Reader's IsMediaSeekable[1] will return false when playing video files inside APP package (FTU case) since it checks IsTransportSeekable() not media content itself. I think we should change it to check mDemuxer->CanSeek() only. About seekability, Reader should only take care of content part. For transport part, it should be known from IsTransportSeekable() in [2]. [1]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/MP4Reader.cpp?from=MP4Reader.cpp&case=true#592 [2]https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoder.cpp#1443
Comment on attachment 8601870 [details] [diff] [review] Change-MP4Reader-s-IsMediaSeekable-to-only-check-demuxer.patch I don't think we can drop the IsTransportSeekable() check. Chris Pearce can answer that. mDemuxer->CanSeek() doesn't return anything useful anyway so it could just be replaced with true.
Attachment #8601870 -
Flags: review?(ajones) → review?(cpearce)
Comment 9•9 years ago
|
||
Comment on attachment 8601870 [details] [diff] [review] Change-MP4Reader-s-IsMediaSeekable-to-only-check-demuxer.patch Review of attachment 8601870 [details] [diff] [review]: ----------------------------------------------------------------- I think this is correct. In cases where the transport is unseekable but the media is, HTMLMediaElement::Seek() should be clamping the seek target to inside the buffered ranges. So we should not be asked to seek to a seek target we can't reach.
Attachment #8601870 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 10•9 years ago
|
||
Carry r+ from cpearce.
Attachment #8601870 -
Attachment is obsolete: true
Attachment #8602601 -
Flags: review+
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #9) > Comment on attachment 8601870 [details] [diff] [review] > Change-MP4Reader-s-IsMediaSeekable-to-only-check-demuxer.patch > > Review of attachment 8601870 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think this is correct. > > In cases where the transport is unseekable but the media is, > HTMLMediaElement::Seek() should be clamping the seek target to inside the > buffered ranges. So we should not be asked to seek to a seek target we can't > reach. Thanks for the review!
Keywords: checkin-needed
Assignee | ||
Comment 12•9 years ago
|
||
Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=849313bdb697
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b091b66b50a7
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b091b66b50a7
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•