Closed Bug 1161444 Opened 9 years ago Closed 9 years ago

Change MP4Reader's IsMediaSeekable()

Categories

(Core :: Audio/Video, defect)

37 Branch
defect
Not set
normal

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.
ajones, 
Do you have concerns to check only mDemuxer->CanSeek()?
Flags: needinfo?(ajones)
Blocks: 1160241
Wrong component...
Component: WebRTC: Audio/Video → Video/Audio
(In reply to Randell Jesup [:jesup] from comment #2)
> Wrong component...
oops. Sorry! :-)
(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)
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
Blocks: 1123603
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 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+
Carry r+ from cpearce.
Attachment #8601870 - Attachment is obsolete: true
Attachment #8602601 - Flags: review+
(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
https://hg.mozilla.org/mozilla-central/rev/b091b66b50a7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.