Closed
Bug 1111413
Opened 9 years ago
Closed 9 years ago
Use MediaPromise for seeking
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: mattwoodrow, Assigned: mattwoodrow)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
10.91 KB,
patch
|
bholley
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
49.94 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8536281 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8536282 -
Flags: review?(bobbyholley)
Comment 3•9 years ago
|
||
Comment on attachment 8536281 [details] [diff] [review] Part 1: Use MediaPromise for seeking instead of a callback Review of attachment 8536281 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Thanks for doing this. This patch seems to be a really compelling use-case for static methods that create already-resolved or already-rejected promises (à la Promise.{resolve,reject} in js), since that would remove the need to declare the temporaries here, and allow these sites to just do: return SeekPromise::CreateAndResolve(true); and return SeekPromise::CreateAndReject(rv); Those methods should be trivial to implement in MediaPromise.h, and would make this patch a good deal simpler. Can I persuade you to do that? If so, please do. But you're also welcome to land this patch as-is if you'd prefer (but please file a followup, and I'll try to do it). ::: dom/media/MediaDecoderStateMachine.cpp @@ +2339,4 @@ > { > ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor()); > mWaitingForDecoderSeek = false; > if (NS_FAILED(aResult)) { Add a comment here indicating that we reject in non-failure cases, specifically when we seek twice, referencing MediaSourceReader. ::: dom/media/android/AndroidMediaReader.h @@ +69,5 @@ > > virtual nsresult ReadMetadata(MediaInfo* aInfo, > MetadataTags** aTags); > + virtual nsRefPtr<SeekPromise> > + Seek(int64_t aTime, int64_t aStartTime, int64_t aEndTime, int64_t aCurrentTime); Please mark this and all over the other codec-specific overrides as MOZ_OVERRIDE. ::: dom/media/mediasource/MediaSourceReader.h @@ +54,5 @@ > void OnAudioNotDecoded(NotDecodedReason aReason); > void OnVideoDecoded(VideoData* aSample); > void OnVideoNotDecoded(NotDecodedReason aReason); > > + void OnSeekCompleted(bool aIgnored); You shouldn't need aIgnored with bug 1109954.
Attachment #8536281 -
Flags: review?(bobbyholley) → review+
Comment 4•9 years ago
|
||
Comment on attachment 8536282 [details] [diff] [review] Part 2: Remove RequestSampleCallback Review of attachment 8536282 [details] [diff] [review]: ----------------------------------------------------------------- \o/
Attachment #8536282 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 5•9 years ago
|
||
Fixed review comments, carrying r=bholley.
Attachment #8536281 -
Attachment is obsolete: true
Attachment #8537131 -
Flags: review+
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/99b9c2fca066 https://hg.mozilla.org/integration/mozilla-inbound/rev/7f738423336a
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/99b9c2fca066 https://hg.mozilla.org/mozilla-central/rev/7f738423336a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 8•9 years ago
|
||
Comment on attachment 8536282 [details] [diff] [review] Part 2: Remove RequestSampleCallback Approval Request Comment [Feature/regressing bug #]: MSE [User impact if declined]: Less consistent testing, sites more likely to serve flash video. [Describe test coverage new/current, TBPL]: Landed on m-c, passes mochitests. [Risks and why]: This affects non-MSE media playback, but issues would have turned up by now. [String/UUID change made/needed]: None. This request applies to all patches on this bug.
Attachment #8536282 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
Updated•9 years ago
|
Attachment #8536282 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 9•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/9eb09ba7451a https://hg.mozilla.org/releases/mozilla-aurora/rev/7e2a075d8906
You need to log in
before you can comment on or make changes to this bug.
Description
•