1158916 - Remove unnecessary calls to mReadyStateWatchTarget->Notify()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

Splitting this out of bug 1157797 for further granularity.

Comment 1

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a22899cfc359

Comment 2

[Bobby Holley (:bholley)]

Attachment: Stop manually notifying MediaDecoder::mReadyStateWatchTarget. v1

If we've got our state graph set up properly, the watch target should be
notified automatically whenever anything relevant changes.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

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?

Comment 4

[Bobby Holley (:bholley)]

(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.

Comment 5

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac61509d2b0

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

(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.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ac61509d2b0

Comment 8

[Bobby Holley (:bholley)]

(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

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ccd154afb3c1