Closed Bug 1158916 Opened 5 years ago Closed 5 years ago

Remove unnecessary calls to mReadyStateWatchTarget->Notify()

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(1 file)

Splitting this out of bug 1157797 for further granularity.
If we've got our state graph set up properly, the watch target should be
notified automatically whenever anything relevant changes.
Attachment #8598335 - Flags: review?(jwwang)
Comment on attachment 8598335 [details] [diff] [review]
Stop manually notifying MediaDecoder::mReadyStateWatchTarget. v1

Review of attachment 8598335 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoder.cpp
@@ -1650,5 @@
>  void MediaDecoder::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int64_t aOffset) {
>    if (mDecoderStateMachine) {
>      mDecoderStateMachine->NotifyDataArrived(aBuffer, aLength, aOffset);
>    }
> -  mReadyStateWatchTarget->Notify(); // XXXbholley - Still necessary?

I am worried this will regress bug 1093399. Can you have a test?
Attachment #8598335 - Flags: review?(jwwang) → review+
(In reply to JW Wang [:jwwang] from comment #3)
> I am worried this will regress bug 1093399. Can you have a test?

Good idea! Works fine it seems - I'm not sure why we'd need this, given that the MDSM will automatically trigger UpdateReadyState when mNextFrameStatus gets to a good state.

I'll push when inbound reopens.
(In reply to Bobby Holley (:bholley) from comment #4)
> (In reply to JW Wang [:jwwang] from comment #3)
> > I am worried this will regress bug 1093399. Can you have a test?
> 
> Good idea! Works fine it seems - I'm not sure why we'd need this, given that
> the MDSM will automatically trigger UpdateReadyState when mNextFrameStatus
> gets to a good state.
> 
> I'll push when inbound reopens.

https://hg.mozilla.org/mozilla-central/file/caf25344f73e/dom/html/HTMLMediaElement.cpp#l3592

It is tricky that readyState=HAVE_ENOUGH_DATA also depends on the download speed which is not about mNextFrameStatus. That's why we used to update readyState again when getting some bytes downloaded to re-compute CanPlayThrough() again.
https://hg.mozilla.org/mozilla-central/rev/8ac61509d2b0
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(In reply to JW Wang [:jwwang] from comment #6)
> It is tricky that readyState=HAVE_ENOUGH_DATA also depends on the download
> speed which is not about mNextFrameStatus. That's why we used to update
> readyState again when getting some bytes downloaded to re-compute
> CanPlayThrough() again.

Hm that's a good point - I've backed out that one part.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccd154afb3c1
You need to log in before you can comment on or make changes to this bug.