Last Comment Bug 449363 - Support media attribute of <source> elements
: Support media attribute of <source> elements
Status: RESOLVED FIXED
: dev-doc-complete, testcase
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal with 5 votes (vote)
: mozilla15
Assigned To: Paul Adenot (:padenot)
:
: Maire Reavy [:mreavy]
Mentors:
Depends on: 464405
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-06 00:32 PDT by Biju
Modified: 2012-05-17 07:14 PDT (History)
25 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v0 : Feature implementation and test. (6.11 KB, patch)
2012-05-01 20:20 PDT, Paul Adenot (:padenot)
cpearce: review-
Details | Diff | Splinter Review
v1 : adressed cpearce and bz remarks. (9.09 KB, patch)
2012-05-07 17:20 PDT, Paul Adenot (:padenot)
cpearce: review-
Details | Diff | Splinter Review
v2 - moved the layout flush to the beginning of the function, and cpearce's other remarks (10.14 KB, patch)
2012-05-07 22:19 PDT, Paul Adenot (:padenot)
bzbarsky: review+
l10n: feedback+
Details | Diff | Splinter Review
v3 - Addresse last comments (10.33 KB, patch)
2012-05-09 15:37 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
v3 - the right patch (10.03 KB, patch)
2012-05-09 18:28 PDT, Paul Adenot (:padenot)
cpearce: review+
Details | Diff | Splinter Review
(Added name a commit message). (10.08 KB, patch)
2012-05-14 17:58 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review

Description Biju 2008-08-06 00:32:37 PDT
Support <source> element in <video>


test:
1. Goto URL http://lachy.id.au/dev/markup/tests/html5/video/004.html

Result:-
User is not seeing video

Expected:-
User should see video
Comment 1 cajbir (:cajbir) 2008-08-06 00:51:32 PDT
Limited functionality of <source> is actually supported. The problem here is that mime types with the codecs parameter are not. So it is not finding the Ogg decoder due to not understanding the mime type.
Comment 2 Matthew Gregan [:kinetik] 2009-02-25 04:40:29 PST
Implementation of canPlayType (bug 469247) also gave us support for parsing the codec potion of the MIME type, so this is fixed.  The linked testcase doesn't actually work, though, because the media URIs are 404s now.
Comment 3 cajbir (:cajbir) 2009-02-25 04:51:59 PST
Note that we don't currently support the media attribute of <source> however.
Comment 4 Matthew Gregan [:kinetik] 2009-02-25 04:57:04 PST
Oh yeah.
Comment 5 Matthew Gregan [:kinetik] 2010-06-24 17:38:04 PDT
When this bug is fixed, the following test should pass:
(Note that the tests current sniff the user agent for Firefox--hopefully that'll be fixed soon.)

http://test.w3.org/html/tests/submission/Microsoft/video/video_007.htm
Comment 6 Paul Adenot (:padenot) 2012-04-30 15:09:37 PDT
I've started researching on how to fix that.

Seems that I need to instantiate a CSSParser, call its |ParseMedialList|, giving it the |media| attribute value (i.e. a string). Then I get a |nsMediaList|, on which I can call |Matches|, giving it the nsPresContext, that we can find by doing |OwnerDoc()->GetShell()->GetPresContext()|.

Then we have a bool, and we choose to use that resource or not.

Does that seems reasonable ? I've never done that before, so I need feedback.
Comment 7 Paul Adenot (:padenot) 2012-04-30 15:20:26 PDT
(In reply to Matthew Gregan [:kinetik] from comment #5)
> When this bug is fixed, the following test should pass:
> (Note that the tests current sniff the user agent for Firefox--hopefully
> that'll be fixed soon.)
> 
> http://test.w3.org/html/tests/submission/Microsoft/video/video_007.htm

This testcase seems completely unrelated, though.
Comment 8 Matthew Gregan [:kinetik] 2012-04-30 15:45:47 PDT
The testcases have changed since 2010, unfortunately.  video_008.htm uses the media attribute; I didn't check to see if any other the others do.
Comment 9 Paul Adenot (:padenot) 2012-05-01 20:20:08 PDT
Created attachment 620174 [details] [diff] [review]
Patch v0 : Feature implementation and test.

Chris, I'm not too sure for the reviewer, here, feel free to point me to a more adequate person if needed.

We don't pass the test mentioned in comment 8, because it specifies |type="application/octet-stream"|, and we don't sniff the media content (yet). If we put an appropriate type instead (one that we support) or no type, the test succeeds.
Comment 10 Chris Pearce (:cpearce) 2012-05-02 22:03:35 PDT
Comment on attachment 620174 [details] [diff] [review]
Patch v0 : Feature implementation and test.

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

Looks pretty good, almost there. :) Just a few things for you to fix up, and I'd like to get Boris Zbarsky to look over this too, since he knows more about media queries than I.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +894,5 @@
> +    if (child->GetAttr(kNameSpaceID_None, nsGkAtoms::media, media) && !media.IsEmpty()) {
> +      nsCSSParser cssParser;
> +      nsRefPtr<nsMediaList> mediaList(new nsMediaList());
> +      cssParser.ParseMediaList(media, NULL, 0, mediaList, false);
> +      if (! mediaList->Matches(OwnerDoc()->GetShell()->GetPresContext(), NULL)) {

s/! mediaList/!mediaList/

Not sure if you need to null check GetShell() and GetPresContext() here too, bz will know.

We're supposed to dispatch an AsyncSourceError here too, and we should call ReportLoadError with an appropriate string to help developers figure out why their supported resources aren't playing.

::: content/html/content/src/nsHTMLSourceElement.cpp
@@ +117,5 @@
>  
> +NS_IMETHODIMP
> +nsHTMLSourceElement::GetMedia(nsAString& aValue)
> +{
> +  nsresult rv = GetAttr(kNameSpaceID_None, nsGkAtoms::media, aValue);

Does this return NS_OK if aValue is empty, i.e. if the media attribute hasn't been set? If GetAttr() returns NS_ERROR_FAILURE if the media attribute hasn't been set, then we should set aValue="all" return NS_OK, rather than returning an error... Can you check please?

::: content/media/test/test_source_media.html
@@ +7,5 @@
> +  <script type="text/javascript" src="manifest.js"></script>
> +</head>
> +<body>
> +<video preload="metadata">
> +  <source src="small-shot.ogg" media="not all">

You should be using getPlayableVideo(gSomethingOrOtherTests) here instead of assigning an arbitrary video; if ogg or webm are disable, these won't work. i.e. please dynamically generate these video elements with their sources determined by getPlayableVideo().

To differentiate between them in your tests, you can append "?pass", "?fail" to the source elements src attributes, and search for that in the currentSrc, rather than just searching for seek.webm.

Other than that, the test looks fine.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-05-03 18:03:11 PDT
> Not sure if you need to null check GetShell() and GetPresContext() here too, bz will know.

You need to null-check GetShell(), but if the shell is not null there is no need to null-check the prescontext.

What's the expected behavior if there's no shell?  That corresponds to an iframe that's display:none... or that just hasn't had a frame constructed yet.  Which is a bigger problem: doing this without flushing frames (or maybe even layout, for the size-dependent queries) is racy.  Is there a spec for how this should work?  What happens with a <video> using an <src @media> inside an iframe when the iframe's size changes?
Comment 12 Paul Adenot (:padenot) 2012-05-03 19:20:55 PDT
Spec : www.whatwg.org/html#dom-source-media. The spec does not say much, though.

If we are in an iframe that is in display:none, our case differ because we can output sound, hence produce a perceptible result whereas the author probably does not want anything to happen. This is debatable. 

Is there a way to get notified when the layout is done, to be able to select the appropriate resource in a non racy manner?

If we resize an iframe that contains a video, I'm not sure what to do. Choosing another resource will provoke a pause in the playback (we need time to buffer for a bit and to instantiate the right decoder, etc.), which is not desirable. When we get DASH [1] support, it may be appropriate to ask for an other resource that fit the new size of the iframe better, but this has nothing to do with media queries, I suppose.

Reading the spec : 

> Dynamically modifying a source element and its attribute when the element is already 
> inserted in a video or audio element will have no effect. To change what is playing,
> just use the src attribute on the media element directly, possibly making use of the 
> canPlayType() method to pick from amongst available resources. Generally, manipulating
> source elements manually after the document has been parsed is an unncessarily 
> complicated approach.

This could imply that |media| attribute is only relevant when the media element is put in the page, and that <source> element should not be used after that point. Since the spec recommends the use of |canPlayType()|, it seems reasonable to think of using |matchMedia()| to get the appropriate resource instead (or to call |load()| on the media element to ask for a new resource to be chosen). That would delegate that question to authors.

[1] : https://en.wikipedia.org/wiki/Dynamic_Adaptive_Streaming_over_HTTP
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2012-05-03 22:39:49 PDT
OK.

So is the code being modified running from a place where it's safe to run script?  If so, you _could_ just flush layout to make sure it's up to date...

There is no way to really get notified when the layout is done right now.
Comment 14 Paul Adenot (:padenot) 2012-05-03 23:12:31 PDT
The code is running in the main thread, it is in content/html/content/src/nsHTMLMediaElement.cpp. How can I flush layout ?
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2012-05-03 23:22:21 PDT
That wasn't a question about threads.  It was a question about what's up the callstack from your code and whether it can deal with arbitrary script running (which can happen during the layout flush).

You can flush using nsIDocument::FlushPendingNotifications.  Probably want to Flush_Style in your case.
Comment 16 Paul Adenot (:padenot) 2012-05-07 17:20:42 PDT
Created attachment 621798 [details] [diff] [review]
v1 : adressed cpearce and bz remarks.

Chris, according to https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#5457, GetAttr truncates the string, so it is safe to call only empty. I've updated the code to reflect that. I've also addressed your other comments.

Boris, here is the relevant call stack from this piece of code : http://pastebin.mozilla.org/1624294. 

I've been advised by dholbert to check if it was safe to call script using the |nsContentUtils::IsSafeToRunScript()| helper, which returns |true| when called in gdb when stopped on a breakpoint just before calling |nsMediaList::Matches()|, so I suppose it's okay.
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2012-05-07 18:34:16 PDT
That callstack is coming in off the event loop, so is certainly safe to do flushes from.

At least as long as:

1)  The media code itself can deal with script running here.
2)  We always enter this code via that one callstack.
Comment 18 Chris Pearce (:cpearce) 2012-05-07 21:40:20 PDT
(In reply to Boris Zbarsky (:bz) from comment #17)
> That callstack is coming in off the event loop, so is certainly safe to do
> flushes from.
> 
> At least as long as:
> 
> 1)  The media code itself can deal with script running here.

The code here can't (yet) handle script running... In this loop we're iterating over the <media> element's <source> children, and any script that runs here could remove any child from the media element, potentially making |child| a dangling pointer.

So rather than checking that the media element's children are still valid while iterating over them, it's probably easiest to just flush layout once when we enter LoadFromSourceChildren(), before we start iterating over the children. This also means we're not flusing layout multiple times, in the case where multiple child <source> elements have a @media attribute.

> 2)  We always enter this code via that one callstack.

We only ever enter that code from that callback (it's a "synchronous section", as per the media load algorithm spec). So this should be OK.
Comment 19 Chris Pearce (:cpearce) 2012-05-07 21:45:09 PDT
Comment on attachment 621798 [details] [diff] [review]
v1 : adressed cpearce and bz remarks.

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

Almost there!

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +891,5 @@
> +      nsCSSParser cssParser;
> +      nsRefPtr<nsMediaList> mediaList(new nsMediaList());
> +      cssParser.ParseMediaList(media, NULL, 0, mediaList, false);
> +      nsRefPtr<nsIPresShell> presShell = OwnerDoc()->GetShell();
> +      if (presShell) {

Unless bz disagrees, move the flush layout to the start of this function.

::: content/media/test/test_source_media.html
@@ +16,5 @@
> +      SimpleTest.finish();
> +    }
> +  }
> +  var v = document.createElement('video');
> +  v.innerHTML = "<source src=\"" + getPlayableVideo(gSmallTests).name + "?fail\" media=\"not all\">" +

You need to handle the case where getPlayableVideo(gSmallTests) returns no result. For example how we handle it here [ http://mxr.mozilla.org/mozilla-central/source/content/media/test/test_closing_connections.html?force=1#36 ] by just aborting the test with a todo().

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +144,5 @@
>  MediaLoadInvalidURI=Invalid URI. Load of media resource %S failed.
>  # LOCALIZATION NOTE: %1$S is the media resource's format/codec type (basically equivalent to the file type, e.g. MP4,AVI,WMV,MOV etc), %2$S is the URL of the media resource which failed to load.
>  MediaLoadUnsupportedType=Specified "type" of "%1$S" is not supported. Load of media resource %2$S failed.
> +# LOCALIZATION NOTE: %1$S is the "media" attribute value of the <source> element. It is a media query. %2$S is the URL of the media resource which failed to load.
> +MediaLoadSourceMediaNotMatched=Specified "media" of "%1$S" does not match the environment. Load of media resource %2$S failed.

Specified "media" attribute of "%1$S"...
(insert "attribute")
Comment 20 Paul Adenot (:padenot) 2012-05-07 22:19:57 PDT
Created attachment 621883 [details] [diff] [review]
v2 - moved the layout flush to the beginning of the function, and cpearce's other remarks
Comment 21 Chris Pearce (:cpearce) 2012-05-07 22:32:43 PDT
Comment on attachment 621883 [details] [diff] [review]
v2 - moved the layout flush to the beginning of the function, and cpearce's other remarks

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

::: content/media/test/test_source_media.html
@@ +23,5 @@
> +    }
> +    return media;
> +  }
> +  var v = document.createElement('video');
> +  v.innerHTML = "<source src=\"" + getPlayableVideoWrapper().name + "?fail\" media=\"not all\">" +

That's not what I meant. You're still running the test if there's no supported media. Plus if getPlayableVideoWrapper() returns null, this will throw an exception.

If the result of getPlayableVideo(gSmallTests) is null, don't run the test, i.e. don't create the video element, and don't call SimpleTest.waitForExplicitFinish().

You should test your patch by setting the prefs media.ogg.enabled and media.webm.enabled to false and running your test.

@@ +41,5 @@
> +    ok(/pass/.test(e.target.currentSrc),
> +      "If no media attribute is specified, it defaults to \'all\'.")
> +    notifyFinished();
> +  });
> +  console.log("Length : "+ document.getElementsByTagName('source').length);

Better to not add unnecessary logging in tests.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2012-05-07 22:42:34 PDT
Comment on attachment 621883 [details] [diff] [review]
v2 - moved the layout flush to the beginning of the function, and cpearce's other remarks

>+++ b/content/html/content/src/nsHTMLMediaElement.cpp
> void nsHTMLMediaElement::LoadFromSourceChildren()
>+  OwnerDoc()->FlushPendingNotifications(Flush_Style);

This could probably be made a slight bit more efficient like so:

  nsIDocument* parentDoc = OwnerDoc()->GetParentDocument();
  if (parentDoc) {
    parentDoc->FlushPendingNotifications(Flush_Layout);
  }

since you don't actually care about flushing style on your own document; you just want its viewport to have the right size, right?

>+      nsRefPtr<nsIPresShell> presShell = OwnerDoc()->GetShell();

That can just be nsIPresShell*.

>+++ b/content/html/content/src/nsHTMLSourceElement.cpp
>+nsHTMLSourceElement::GetMedia(nsAString& aValue)
>+{
>+  GetAttr(kNameSpaceID_None, nsGkAtoms::media, aValue);
>+  if (aValue.IsEmpty()) {
>+    aValue.AssignASCII("all");

I believe this is not what the spec says to do.  The spec says:

  The IDL attributes src, type, and media must reflect the respective
  content attributes of the same name.

which means that .media should return the same thing as .getAttribute("media").  If there are tests in the W3C test suite that don't match that, they should be fixed or the spec amended.  If there are no tests that check this behavior, there probably should be some.

So you really just want to use NS_IMPL_STRING_ATTR here.

Our own tests should check the behavior here too; use reflect.js for that.

>+++ b/dom/locales/en-US/chrome/dom/dom.properties

At one point we needed to change the key when changing the string, but maybe we've fixed that since then.  Axel, is that still needed?

r=me with the above nits fixed.
Comment 23 Paul Adenot (:padenot) 2012-05-07 22:58:48 PDT
Comment 10 says that we should return "all" and the spec could be interpreted in two ways :

Regarding <source>'s media attribute :
> The default, if the media attribute is omitted, is "all", meaning that by default the media 
> resource is suitable for all media.

Regarding reflection :
> In general, on getting, if the content attribute is not present, the IDL attribute must act 
> as if the content attribute's value is the empty string; and on setting, if the content 
> attribute is not present, it must first be added. 

That could either mean to return "all", or to act as if "all" was specified, I'm not enough familiar with the spec writing style to decide. The current test for this feature checks that we return "all" using the property if the attribute is not set.

We should probably email the whatwg to get that clarified.
Comment 24 Axel Hecht [:Pike] 2012-05-07 23:12:56 PDT
Comment on attachment 621883 [details] [diff] [review]
v2 - moved the layout flush to the beginning of the function, and cpearce's other remarks

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

We still need a key change for strings that change, thus giving this a f-.

Today, we have two kinds of string changes:
- we want/need localizer action
- we assume that localizers will never need to look at this (typos in English, ellipsis change, etc)

We may add a third level in the future, "The old localization will still work, but you want to cross check your string against the new one." This one would probably be in that bucket, as the old string still works, but the new string is clearly better.

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +142,5 @@
>  MediaLoadHttpError=HTTP load failed with status %1$S. Load of media resource %2$S failed.
>  # LOCALIZATION NOTE: %S is the URL of the media resource which failed to load.
>  MediaLoadInvalidURI=Invalid URI. Load of media resource %S failed.
>  # LOCALIZATION NOTE: %1$S is the media resource's format/codec type (basically equivalent to the file type, e.g. MP4,AVI,WMV,MOV etc), %2$S is the URL of the media resource which failed to load.
> +MediaLoadUnsupportedType=Specified "type" attribute of "%1$S" is not supported. Load of media resource %2$S failed.

This needs a new key, still.
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2012-05-07 23:29:43 PDT
> Comment 10 says that we should return "all"

Then the spec needs changing.

> and the spec could be interpreted in two ways

The relevant part of the spec is right here:

  If a reflecting IDL attribute is a DOMString attribute but doesn't fall into any of the
  above categories, then the getting and setting must be done in a transparent,
  case-preserving manner.

which is the actual conformance requirement.  As in, it just calls getAttribute.

> The current test for this feature checks that we return "all" using the property if the
> attribute is not set.

Does it check that "all" is returned if the attribute is explicitly set to ""?

Agreed that mailing whatwg is desirable here.
Comment 26 Paul Adenot (:padenot) 2012-05-09 15:37:29 PDT
Created attachment 622549 [details] [diff] [review]
v3 - Addresse last comments

Here is a updated patch, adding proper test handling, working when certain media types are disabled, new behavior for the media attribute (which now reflects the content, as answered on the whatwg mailing list), and tests for this behavior, using reflect.js.

Also updated the key for the changed string as per comment 24.
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2012-05-09 18:25:20 PDT
Comment on attachment 622549 [details] [diff] [review]
v3 - Addresse last comments

This seems to be missing the reflect.js bits and still has the "return 'all' if not set" behavior.
Comment 28 Paul Adenot (:padenot) 2012-05-09 18:28:07 PDT
Created attachment 622599 [details] [diff] [review]
v3 - the right patch

Here you go, I picked the patch in the wrong tree last time.
Comment 29 Chris Pearce (:cpearce) 2012-05-09 21:22:59 PDT
Comment on attachment 622599 [details] [diff] [review]
v3 - the right patch

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

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +897,5 @@
> +      nsCSSParser cssParser;
> +      nsRefPtr<nsMediaList> mediaList(new nsMediaList());
> +      cssParser.ParseMediaList(media, NULL, 0, mediaList, false);
> +      nsIPresShell* presShell = OwnerDoc()->GetShell();
> +      if (presShell && !mediaList->Matches(presShell->GetPresContext(), NULL)) {

bz: I wonder if this should be:
     if (!presShell || !mediaList->Matches(presShell->GetPresContext(), NULL)) {

i.e. should we treat having a null presshell the same as we treat having a presshell's prescontext not match the media list?
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2012-05-09 22:33:00 PDT
That really depends on the behavior we want.  See the first question in comment 11...  The spec doesn't seem to actually say; that should get raised as a spec issue.
Comment 31 Paul Adenot (:padenot) 2012-05-10 17:08:35 PDT
> What happens with a <video> using an <src @media> inside an iframe when the iframe's size 
> changes?

Nothing, we keep playing the same resource (as per discussion on the whatwg mailing list).

> What's the expected behavior if there's no shell?

We should do the same behavior as having a video that is has display: none, right ? (since as I understand it, if we flush the layout, we are assured that we have a frame, so display:none is the only way not to have a shell).
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2012-05-10 17:46:58 PDT
display:none on the video won't give you a null presshell.  display:none on the iframe that the video is in will.

So no, it's not the same behavior as display:none video.
Comment 33 Chris Pearce (:cpearce) 2012-05-10 18:19:29 PDT
Comment on attachment 622599 [details] [diff] [review]
v3 - the right patch

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

(In reply to Boris Zbarsky (:bz) from comment #32)
> display:none on the video won't give you a null presshell.  display:none on
> the iframe that the video is in will.
> 
> So no, it's not the same behavior as display:none video.

OK. We can't get a prescontext to match against if we don't have a presshell, so too bad. People should just Not Do That.
Comment 34 Paul Adenot (:padenot) 2012-05-14 17:58:53 PDT
Created attachment 623889 [details] [diff] [review]
(Added name a commit message).
Comment 35 Ryan VanderMeulen [:RyanVM] 2012-05-15 06:03:32 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b15a3c5dd0e6
Comment 36 Ed Morley [:emorley] 2012-05-16 03:55:40 PDT
https://hg.mozilla.org/mozilla-central/rev/b15a3c5dd0e6
Comment 37 Eric Shepherd [:sheppy] 2012-05-17 07:14:06 PDT
Docs updated:

https://developer.mozilla.org/en/HTML/Element/Source

And mentioned on Firefox 15 for developers.

Note You need to log in before you can comment on or make changes to this bug.