883731 - Refactor HTMLMediaElement to remove ResourceLoaded and improve readyState management

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Remove nsIDOMHTMLMediaElement:: and MediaDecoderOwner:: prefixes

We'll need these enums defined in HTMLMediaElement when nsIDOMHTMLMediaElement goes away completely. Having them there now lets us make the code nicer and removes a lot of references to the obsolete nsIDOMHTMLMediaElement.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Remove ResourceLoaded notifications since they don't make much sense with a media cache

What ResourceLoaded used to do is now handled by UpdateReadyState (setting HAVE_ENOUGH_DATA if the resource loader decides to stop downloading) and DownloadSuspended.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: Make all calls to ChangeReadyState go through UpdateReadyStateForData

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 763483 [details] [diff] [review]
Part 3: Make all calls to ChangeReadyState go through UpdateReadyStateForData

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

::: content/html/content/public/HTMLMediaElement.h
@@ +239,1 @@
>    void ChangeReadyState(nsMediaReadyState aState);

If only UpdateReadyStateForData can call this, make it private.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 3 v2

Previous patch had two bugs:
1) mBegun wasn't reset for a new resource load. That means UpdatePreloadAction would reach this code:
    // We've started a load or are already downloading, and the preload was
    // changed to a state where we buffer less. We don't support this case,
    // so don't change the preload behaviour
and fail to reset the preload action. We need to reset mBegun to false in AbortExistingLoads. I think this is an existing bug that was previously masked in our tests by ResourceLoaded resetting mBegun to false.
2) We would fire loadeddata on every transition from HAVE_METADATA to >= HAVE_CURRENT_DATA. This is incorrect because it should only happen once per resource, i.e. should not happen when seeking completes. Fixed this by adding another flag to track whether loadeddata has been fired.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 764555 [details] [diff] [review]
part 3 v2

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

::: content/html/content/public/HTMLMediaElement.h
@@ +239,1 @@
>    void ChangeReadyState(nsMediaReadyState aState);

Since this isn't called from outside the class, it can have protected visibility.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/acc1c0494f49
https://hg.mozilla.org/integration/mozilla-inbound/rev/78424c3ea6cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/04d60f86b935

Comment 8

[Ed Morley [:emorley]]

This and bug 879717 backed out for causing extremely frequent (but not 100%) assertions on Windows during test_playback_rate.html:
https://tbpl.mozilla.org/php/getParsedLog.php?id=24325130&full=1&branch=mozilla-inbound#error0
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=04d60f86b935

https://hg.mozilla.org/integration/mozilla-inbound/rev/abe93cefa9cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/5199b67ba029
https://hg.mozilla.org/integration/mozilla-inbound/rev/b22438ffe268
https://hg.mozilla.org/integration/mozilla-inbound/rev/bef10ede6c1e

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 1 rebased

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 2 rebased

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 3 rebased

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: MediaDecoder::Resume(true) should not try to start buffering if we're already playing

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 767560 [details] [diff] [review]
Part 4: MediaDecoder::Resume(true) should not try to start buffering if we're already playing

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

It is bad that the caller of StartBuffering() must be aware of StartBuffering()'s internals. I think we're better off moving the if (IsPlaying()){StopPlayback();} block out of StartBuffering() and into the DECODER_STATE_BUFFERING case of MediaDecoderStateMachine::RunStateMachine(), inside the if statement branch where we've decided that we need to keep buffering (i.e. the branch other than the one that calls StartDecoding()). Then there's no chance in future of someone adding a caller to StartBuffering() which hits the same condition, and we won't stop playback unless we need to.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Good call.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4 v2

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=1a29915b92f7

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=370e98e850db

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=c5791a92331e

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This causes intermittent timeouts in test_buffered.html.
https://tbpl.mozilla.org/php/getParsedLog.php?id=24702742&tree=Try&full=1

Comment 20

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

Please have a look when you have time.

Comment 21

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

Per discussion with roc, I will take this bug and continue his work here.

Comment 22

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

Attachment: 883731_part1_remove_ResourceLoaded-v1.patch

Part1 - Remove ResourceLoaded notifications since they don't make much sense with a media cache. Rebased on roc's patch in https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=883731&attachment=767505.

Comment 23

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

Attachment: 883731_part2_remove_unnecessary_update_readyState-v1.patch

Part2 - Remove unnecessary update of readyState and introduce NEXT_FRAME_UNAVAILABLE_SEEKING so that we return HAVE_METADATA while seeking.

Comment 24

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

Attachment: 883731_part3_fix_start_stop_progress-v1.patch

Progress update should be associated with network state transitions. Also centralize network state transition code.

Comment 25

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

Comment on attachment 8494927 [details] [diff] [review]
883731_part2_remove_unnecessary_update_readyState-v1.patch

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3199,5 @@
>    UpdateAudioChannelPlayingState();
>  
>    // Handle raising of "waiting" event during seek (see 4.8.10.9)
>    if (mPlayingBeforeSeek &&
> +      mReadyState < nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA) {

Oops, we should check |mReadyState| here instead of |oldState|.

Comment 26

[Matthew Gregan [:kinetik]]

Comment on attachment 8494927 [details] [diff] [review]
883731_part2_remove_unnecessary_update_readyState-v1.patch

We should probably remove NEXT_FRAME_WAIT_FOR_MSE_DATA, because that was added only to get the behaviour you're now adding/fixing, but was originally specific to MSE.

Comment 27

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

Comment on attachment 8494928 [details] [diff] [review]
883731_part3_fix_start_stop_progress-v1.patch

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3412,5 @@
>  {
>    // TODO:
>    //   the current playback position is equal to the effective end of the media resource.
>    //   See bug 449157.
> +  return mReadyState >= nsIDOMHTMLMediaElement::HAVE_METADATA &&

It looks like we should check |mReadyState| here which is related to playback state.

::: content/media/MediaDecoder.cpp
@@ -138,5 @@
>    }
>  
>    if(aDormant) {
>      // enter dormant state
> -    StopProgress();

Progress notification should be associated with the network state instead of playback state. Dormant state will suspend download and progress timer will be stopped in response to network state transition.

Comment 28

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=80e118973bea
Most green.

Comment 29

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

(In reply to Matthew Gregan [:kinetik] from comment #26)
> We should probably remove NEXT_FRAME_WAIT_FOR_MSE_DATA, because that was
> added only to get the behaviour you're now adding/fixing, but was originally
> specific to MSE.

I guess we can also remove ChangeToHaveMetadata in MediaSourceReader.cpp since the state machine will handle readyState changes while seeking. I will file a following bug to investigate this and figure out what to do.

Comment 30

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8494928 [details] [diff] [review]
883731_part3_fix_start_stop_progress-v1.patch

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

::: content/media/MediaDecoder.h
@@ +688,5 @@
>    PlayState GetState() {
>      return mPlayState;
>    }
>  
> +  // Called by the media element to start timer to update download progress.

It feels like reporting that the download has progressed should be the responsibility of the MediaResource. (I'm not suggesting that you change your patches, just making an observation).

Comment 31

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

Comment on attachment 8494928 [details] [diff] [review]
883731_part3_fix_start_stop_progress-v1.patch

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

::: content/media/MediaDecoder.h
@@ +688,5 @@
>    PlayState GetState() {
>      return mPlayState;
>    }
>  
> +  // Called by the media element to start timer to update download progress.

In fact, I want to move download progress notification to HTMLMediaElement as far as object responsibility is concerned. MediaResource is responsible for downloading data, however, it is media element's responsibility to decide when to send download progress notifications which are DOM events. I would like to keep DOM activities out of MediaResource to reduce coupling. (It doesn't feel me right seeing HTMLMediaElement is referred in MediaResource.)

That said, I haven't had a clear idea how to divide object responsibilities as far as download progress is concerned.

Comment 32

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=80e118973bea
Most green.

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5db9f8fbcaf4
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca140e7557d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/43a1f1f79302

Comment 34

[Carsten Book [:Tomcat]]

sorry had to back this out for failing tests like https://tbpl.mozilla.org/php/getParsedLog.php?id=49086586&tree=Mozilla-Inbound

Comment 35

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

Attachment: 883731_part4_fix_autoplay_activated_before_metadata_loaded-v1.patch

Per spec. autoplay activation should be handled by readyState transition.

Comment 36

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

Comment on attachment 8497180 [details] [diff] [review]
883731_part4_fix_autoplay_activated_before_metadata_loaded-v1.patch

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=94c484750d89
Looks like it didn't break w3c tests.

::: content/html/content/src/HTMLMediaElement.cpp
@@ -3071,5 @@
>  {
>    mDownloadSuspendedByCache = aIsSuspended;
> -  // If this is an autoplay element, we may need to kick off its autoplaying
> -  // now so we consume data and hopefully free up cache space.
> -  CheckAutoplayDataReady();

MediaDecoder will update readyState after notifying SuspendedByCache and autoplay will be activated if necessary during readyState transition. We shouldn't call CheckAutoplayDataReady() manually here.

@@ -3292,5 @@
>    // being paused.
>    return !mPausedForInactiveDocumentOrChannel &&
>           mAutoplaying &&
>           mPaused &&
> -         (mDownloadSuspendedByCache ||

Don't check |mDownloadSuspendedByCache| here which will change the readyState and activate autoplay if necessary.

Comment 37

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=94c484750d89
Looks like it didn't break w3c tests.

Comment 38

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f57e6c8a9bca
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e032e6b7d33
https://hg.mozilla.org/integration/mozilla-inbound/rev/951d22a66a3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/37b7f512963a

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f57e6c8a9bca
https://hg.mozilla.org/mozilla-central/rev/8e032e6b7d33
https://hg.mozilla.org/mozilla-central/rev/951d22a66a3c
https://hg.mozilla.org/mozilla-central/rev/37b7f512963a