Closed Bug 883731 Opened 8 years ago Closed 7 years ago

Refactor HTMLMediaElement to remove ResourceLoaded and improve readyState management

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35

People

(Reporter: roc, Assigned: jwwang)

References

Details

Attachments

(4 files, 9 obsolete files)

883731_part1_remove_ResourceLoaded-v1.patch
25.89 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
883731_part2_remove_unnecessary_update_readyState-v1.patch
9.23 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
883731_part3_fix_start_stop_progress-v1.patch
24.82 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
883731_part4_fix_autoplay_activated_before_metadata_loaded-v1.patch
1.89 KB, patch
cpearce
: review+
Details | Diff | Splinter Review 
No description provided.
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.
Attachment #763406 - Flags: review?(cpearce)
What ResourceLoaded used to do is now handled by UpdateReadyState (setting HAVE_ENOUGH_DATA if the resource loader decides to stop downloading) and DownloadSuspended.
Attachment #763431 - Flags: review?(cpearce)
Attachment #763406 - Flags: review?(cpearce) → review+
Attachment #763431 - Flags: review?(cpearce) → review+
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.
Attachment #763483 - Flags: review?(cpearce) → review+
Attached patch part 3 v2 (obsolete) — — Splinter Review 
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.
Attachment #763483 - Attachment is obsolete: true
Attachment #764555 - Flags: review?(cpearce)
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.
Attachment #764555 - Flags: review?(cpearce) → review+
Attached patch part 1 rebased (obsolete) — — Splinter Review
Attachment #763406 - Attachment is obsolete: true
Attached patch part 2 rebased (obsolete) — — Splinter Review
Attachment #763431 - Attachment is obsolete: true
Attached patch part 3 rebased (obsolete) — — Splinter Review
Attachment #764555 - Attachment is obsolete: true
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.
Attached patch Part 4 v2 (obsolete) — — Splinter Review
Attachment #767560 - Attachment is obsolete: true
Attachment #767560 - Flags: review?(cpearce)
Attachment #768710 - Flags: review?(cpearce)
Attachment #768710 - Flags: review?(cpearce) → review+
Please have a look when you have time.
Flags: needinfo?(jwwang)
Per discussion with roc, I will take this bug and continue his work here.
Assignee: roc → jwwang
Status: NEW → ASSIGNED
Flags: needinfo?(jwwang)
Blocks: 1067041
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.
Attachment #767504 - Attachment is obsolete: true
Attachment #767505 - Attachment is obsolete: true
Attachment #767506 - Attachment is obsolete: true
Attachment #768710 - Attachment is obsolete: true
Part2 - Remove unnecessary update of readyState and introduce NEXT_FRAME_UNAVAILABLE_SEEKING so that we return HAVE_METADATA while seeking.
Progress update should be associated with network state transitions. Also centralize network state transition code.
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 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 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.
Attachment #8494926 - Flags: review?(cpearce)
Attachment #8494927 - Flags: review?(cpearce)
Attachment #8494928 - Flags: review?(cpearce)
(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.
Attachment #8494926 - Flags: review?(cpearce) → review+
Attachment #8494927 - Flags: review?(cpearce) → review+
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).
Attachment #8494928 - Flags: review?(cpearce) → review+
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.
Per spec. autoplay activation should be handled by readyState transition.
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.
Attachment #8497180 - Flags: review?(cpearce)
Attachment #8497180 - Flags: review?(cpearce) → review+
Try: https://tbpl.mozilla.org/?tree=Try&rev=94c484750d89
Looks like it didn't break w3c tests.
Keywords: checkin-needed
Depends on: 1093399
Depends on: 1397708
You need to log in before you can comment on or make changes to this bug.