680321 - Media preload state should reset in resource selection algorithm

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

Attachment: Testcase

The media element's preloadAction should be re-evaluated at the start of the synchronous section in the resource selection algorithm. Otherwise if an existing media element's preload state is changed to a state which requires less buffing and a new load is started, the new load will ignore the new preload state. 

E.g.: (attached testcase)

<script>
var v = document.createElement("video");
v.preload = "auto";
v.controls = "on";
v.src = "http://pearce.org.nz/video/bruce.webm";
document.body.appendChild(v);
v.onloadedmetadata =
  function() {
    v.preload = "none";
    setTimeout(function(){v.src = "http://pearce.org.nz/video/arctic_giant.webm";},1000);
    v.onloadedmetadata = null;
  };
</script>

In this testcase arctic_giant.webm should load assuming preload="none", but currently it loads assuming preload="auto".

This change will make cases like the following to work more intuitively:

<script>
var v = document.createElement("video");
v.preload = "auto";
v.src = "video.webm"; // Currently we'll load assuming preload="auto".
v.preload = "none"; // We should really load assuming preload="none"!
</script>

Philip Jägenstedt pointed out this bug at:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-August/032953.html

Comment 1

[David Seifried (:dseif)]

Ill attempt this if someone can assign it to me.

Comment 2

[David Seifried (:dseif)]

Attachment: Initial patch looking for feedback. No tests.

I took Chris' advice and altered the SelectResource method ensuring that a preload state that requires less buffering is not ignored.  I have not written any tests yet but I'm just throwing this up for some feedback to make sure I'm on the right path.

Comment 3

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

Comment on attachment 591687 [details] [diff] [review]
Initial patch looking for feedback. No tests.

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

Sorry for slow feedback. Long weekend here.

Can you look a the testcase attached here in the bug and add a similar test to content/media/test/test_preload_actions.html?

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ -889,5 @@
>        nextAction = static_cast<PreloadAction>(preloadDefault);
>      }
>    }
>  
> -  if ((mBegun || mIsRunningSelectResource) && nextAction < mPreloadAction) {

So mBegun is true after we've chosen a resource and started to download and decode it. We don't want to support reductions in the amount of buffering after decoding has begun at the current time; that should be the subject of another bug.

So instead of doing what you're doing now, create a new boolean flag mHaveQueuedSelectResource, and use that in QueueSelectResourceTask instead of using mIsRunningSelectResource. This will prevent multiple calls to SelectResource being queued concurrently. Then call UpdatePreloadAction() in SelectResource, but set mIsRunningSelectResource to true in SelectResource() *after* calling UpdatePreloadAction().

Then the preload action will be updated before we start trying to load the resource, so if it's changed after load() is called on the media element, but before the load's synchronous section is run, the new load will reflect the changed preload state.

If that doesn't make sense (or work!), let me know. :)

Comment 4

[David Seifried (:dseif)]

Attachment: Patch with tests

Made all the changes Chris suggested ( including tests ).  I ran the tests with mine included and they are still passing. Looking forward to review :)

Comment 5

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

Comment on attachment 593717 [details] [diff] [review]
Patch with tests

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

Thanks! We're getting there!

Can you also add a commit message of the form "Bug XXXXXX - Describe what the patch does in active voice."

::: content/html/content/public/nsHTMLMediaElement.h
@@ +735,5 @@
>    // True when we've got a task queued to call SelectResource(),
>    // or while we're running SelectResource().
>    bool mIsRunningSelectResource;
>  
> +  bool mHaveQueuedSelectResource;

Add a comment similar to the ones above describing what this boolean is used for please.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +682,5 @@
>    // AddRemoveSelfReference, since it must still be held
>    DispatchAsyncEvent(NS_LITERAL_STRING("loadstart"));
>  
> +  UpdatePreloadAction();
> +  mIsRunningSelectResource = true;

Add a comment explaining why we *must* delay setting mIsRunningSelectResource to true here, so that in future we aren't mystified by why this is needed.

@@ +894,5 @@
>        nextAction = static_cast<PreloadAction>(preloadDefault);
>      }
>    }
>  
> +  if ((mBegun || mHaveQueuedSelectResource) && nextAction < mPreloadAction) {

You shouldn't need to make this change?

The idea of adding mHaveQueuedSelectResource and delaying setting mIsRunningSelectResource to true until after we've updated the preload action is so that this condition doesn't bail out of the preload state update too early, thus losing the state change. If you revert this change we still fix the bug right?

::: content/media/test/test_preload_actions.html
@@ +531,5 @@
> +      is(v.preload === "auto", true, "(19) preload is initially auto");
> +      setTimeout(function() {
> +        // set preload state to none and switch video sources
> +        v.preload="none";
> +        v.src = "http://pearce.org.nz/video/arctic_giant.webm";

Even though this is preload="none", best to stick resource that the project will always control: something checked into the code can be guaranteed to exist. Otherwise if I deleted that file from my server, and a regression was introduced this test may not fail since the resource is 404 and won't load anyway.

Use test.name+"?something" as the video's src attribute. That should fool our caches into thinking it's a different resource.

@@ +552,2 @@
>      }
> +  }

Can you also add the other testcase I mentioned in comment 0, i.e. something like:

var v = document.createElement("video");
v.preload = "auto";
v.src = "video.webm"; // Currently we'll load assuming preload="auto".
v.preload = "none"; // We should really load assuming preload="none"!
</script>

Comment 6

[David Seifried (:dseif)]

> @@ +894,5 @@
> >        nextAction = static_cast<PreloadAction>(preloadDefault);
> >      }
> >    }
> >  
> > +  if ((mBegun || mHaveQueuedSelectResource) && nextAction < mPreloadAction) {
> 
> You shouldn't need to make this change?
> 
> The idea of adding mHaveQueuedSelectResource and delaying setting
> mIsRunningSelectResource to true until after we've updated the preload
> action is so that this condition doesn't bail out of the preload state
> update too early, thus losing the state change. If you revert this change we
> still fix the bug right?

I ran into this issue for the better part of last night and it seems that if we leave it the way it originally was, it fails test 13 in content/media/test/test_preload_actions.html.  The code that I added in was my work around to encountering that issue.  I'm going to look into this more tonight to see if I can figure out a better solution.  Any ideas?

Comment 7

[David Seifried (:dseif)]

I fixed up everything you mentioned Chris as well as what we spoke about on IRC, but we are still failing test 13.  I remember you mentioned that test 13 was written incorrectly, mind refreshing my mind on that?

Comment 8

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

Test 13 is written assuming incorrect behaviour. That testcase should load with preload=none, but it's assuming the test loads with preload=metadata. This is basically the second example I pointed out in comment 0. Please change the test to verify that the load loads with preload=none; you can use the same basic approach as testcase 6 for that.

Comment 9

[David Seifried (:dseif)]

Attachment: Patch v2 with tests

Made all of the request's that Chris had. Unit tests in content/media/tests/ are all passing now.

Comment 10

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

Comment on attachment 598064 [details] [diff] [review]
Patch v2 with tests

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

Almost there! Just a few little typos to fix, and we can land this.

::: content/html/content/public/nsHTMLMediaElement.h
@@ +456,5 @@
>     */
>    void SelectResource();
>  
>    /**
> +   * A wrapper function that allows us to cleanly reset flag after a call

s/flag/flags/

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +663,5 @@
> +{
> +  SelectResource();
> +  mIsRunningSelectResource = false;
> +  mHaveQueuedSelectResource = false;
> +}

There should be one new line between functions.

@@ +687,5 @@
> +  // so that we don't lose our state change by bailing out of the preload
> +  // state update
> +  UpdatePreloadAction();
> +  mIsRunningSelectResource = true;
> +  

Don't add trailing spaces please.

@@ +890,5 @@
>        } else if (attr == nsHTMLMediaElement::PRELOAD_ATTR_METADATA) {
>          nextAction = nsHTMLMediaElement::PRELOAD_METADATA;
>        } else if (attr == nsHTMLMediaElement::PRELOAD_ATTR_NONE) {
>          nextAction = nsHTMLMediaElement::PRELOAD_NONE;
> +      } 

Don't add trailing spaces please.

@@ +928,5 @@
>        // resume the load. We'll pause the load again after we've read the
>        // metadata.
>        ResumeLoad(PRELOAD_METADATA);
>      }
> +  } 

Don't add trailing spaces please.

::: content/media/test/test_preload_actions.html
@@ +392,3 @@
>      function(e) {
>        var v = e.target;
> +      is(v._gotLoadStart, true, "(6) Must get loadstart.");

Change all "(6)" to "(13)".

Comment 11

[David Seifried (:dseif)]

Attachment: Patchv3 with tests

Fixed styling issues and spelling mistake.  Thank's a lot for helping me with this Chris!

Comment 12

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

Comment on attachment 598114 [details] [diff] [review]
Patchv3 with tests

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

Great, thank very much. r=cpearce

Comment 13

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

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5386d7930bf

Comment 14

[Ed Morley [:emorley]]

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