Closed Bug 449363 Opened 16 years ago Closed 12 years ago

Support media attribute of <source> elements

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: BijuMailList, Assigned: padenot)

References

Details

(Keywords: dev-doc-complete, testcase)

Attachments

(1 file, 5 obsolete files)

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
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.
Depends on: 464405
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.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Note that we don't currently support the media attribute of <source> however.
Oh yeah.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Support <source> element in <video> → Support media attribute of <source> elements
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
Keywords: dev-doc-needed
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.
(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.
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.
Assignee: nobody → paul
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.
Attachment #620174 - Flags: review?(cpearce)
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.
Attachment #620174 - Flags: review?(cpearce)
Attachment #620174 - Flags: review?(bzbarsky)
Attachment #620174 - Flags: review-
> 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?
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
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.
The code is running in the main thread, it is in content/html/content/src/nsHTMLMediaElement.cpp. How can I flush layout ?
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.
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.
Attachment #620174 - Attachment is obsolete: true
Attachment #621798 - Flags: review?(cpearce)
Attachment #620174 - Flags: review?(bzbarsky)
Attachment #621798 - Flags: review?(bzbarsky)
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.
(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 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")
Attachment #621798 - Flags: review?(cpearce) → review-
Attachment #621798 - Attachment is obsolete: true
Attachment #621883 - Flags: review?(cpearce)
Attachment #621798 - Flags: review?(bzbarsky)
Attachment #621883 - Flags: review?(bzbarsky)
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.
Attachment #621883 - Flags: review?(cpearce) → review-
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.
Attachment #621883 - Flags: review?(cpearce)
Attachment #621883 - Flags: review?(bzbarsky)
Attachment #621883 - Flags: review-
Attachment #621883 - Flags: review+
Attachment #621883 - Flags: feedback?(l10n)
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 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.
Attachment #621883 - Flags: feedback?(l10n) → feedback+
> 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.
Attached patch v3 - Addresse last comments (obsolete) — Splinter Review
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.
Attachment #621883 - Attachment is obsolete: true
Attachment #622549 - Flags: review?(cpearce)
Attachment #621883 - Flags: review?(cpearce)
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.
Attached patch v3 - the right patch (obsolete) — Splinter Review
Here you go, I picked the patch in the wrong tree last time.
Attachment #622549 - Attachment is obsolete: true
Attachment #622599 - Flags: review?(cpearce)
Attachment #622549 - Flags: review?(cpearce)
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?
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.
> 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).
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 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.
Attachment #622599 - Flags: review?(cpearce) → review+
Attachment #622599 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b15a3c5dd0e6
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/b15a3c5dd0e6
Status: REOPENED → RESOLVED
Closed: 15 years ago12 years ago
Resolution: --- → FIXED
Docs updated:

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

And mentioned on Firefox 15 for developers.
See Also: → 1325053
You need to log in before you can comment on or make changes to this bug.