Closed Bug 462455 Opened 13 years ago Closed 12 years ago

Appending <source> child element to <video> element does not initiate media load

Categories

(Core :: Audio/Video, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: kinetik, Assigned: cpearce)

References

Details

(Keywords: fixed1.9.1, Whiteboard: [tb3needs])

Attachments

(1 file, 8 obsolete files)

Starting with a <video> element with no src attribute or <source> child and attempting to load one later by appending a <source> child element does not initiate media load.  Setting the src attribute works as expected.
Attached patch mochitest (obsolete) — Splinter Review
I mentioned this bug to roc and he said it should be blocking; requesting b1.9.1.
Flags: blocking1.9.1?
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Flags: blocking1.9.1? → blocking1.9.1+
Depends on: 465458
How we doing here?  Been a while since this blocker has been updated.
Should be relatively easy to fix.
Assignee: kinetik → chris
Override nsHTMLSourceElement::BindToTree() to asynchronously ensure that the owning media element starts a load. The spec doesn't specify what to do if a child source element is removed from the media element, so I don't do anything on unbind. Includes mochitest based on Matthew Gregan's original mochitest.
Attachment #345624 - Attachment is obsolete: true
Attachment #360223 - Flags: review?(roc)
+  is(media.currentSrc, "http://localhost:8888/tests/content/media/video/test/320x240.ogv");

I don't think we should depend on the exact URL including port etc. Just check that the last part is "/320x240.ogv".

Otherwise the patch looks good. We don't want to do this event when the parent element itself has only just been bound to the tree and started loading, but I think that works OK here because if the parent has started loading, networkState will not be EMPTY.
Depends on: 476731
Attached patch Patch v2 (obsolete) — Splinter Review
Similar to v1, except:
* Fixes livelock that showed up in nsWaveDecoder. Caused by us not being able to distinguish reading 0 bytes due to EOF with reading 0 bytes due to slow incoming data. ReadAll() in nsWaveDecoder.cpp would get stuck in an infinite loop if shutdown came in while it was reading.
* Added extra mochitest to ensure we don't start loading <video> until </video> is parsed.
* Enabled all tests in test_media_selection.html.
Attachment #360223 - Attachment is obsolete: true
Attachment #361206 - Flags: superreview?(roc)
Attachment #361206 - Flags: review?(chris.double)
Attachment #360223 - Flags: review?(roc)
Comment on attachment 361206 [details] [diff] [review]
Patch v2

+PRBool nsHTMLMediaElement::ShouldLoadOnSourceBind()
+{
+  // Binding a source element to a media element could trigger a new load.
+  // See HTML spec, '4.8.9 The source element' for conditions:
+  // http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#the-source-element
+  // Note: we must not start a load if we're in nsHTMLMediaElement::BindToTree(),
+  // that will trigger a load when it completes.
+  return IsInDoc() &&

You don't need to check IsInDoc here. A child <source> element can't bind to the tree while its parent is not in the tree.

+  nsCOMPtr<nsHTMLMediaElement> media = do_QueryInterface(aParent);

nsHTMLMediaElement has no IID so it's not really clear what this is doing. You probably need to give it one.
(In reply to comment #8)

> +  nsCOMPtr<nsHTMLMediaElement> media = do_QueryInterface(aParent);
> 
> nsHTMLMediaElement has no IID so it's not really clear what this is doing. You
> probably need to give it one.

I'm unsure how to add an IID here. Do you know where I can find an example?
Actually a better idea is probably to add a bit to IsNodeOfType in nsINode.
(In reply to comment #8)
> (From update of attachment 361206 [details] [diff] [review])
> +PRBool nsHTMLMediaElement::ShouldLoadOnSourceBind()
> +{
> +  // Binding a source element to a media element could trigger a new load.
> +  // See HTML spec, '4.8.9 The source element' for conditions:
> +  //
> http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#the-source-element
> +  // Note: we must not start a load if we're in
> nsHTMLMediaElement::BindToTree(),
> +  // that will trigger a load when it completes.
> +  return IsInDoc() &&
> 
> You don't need to check IsInDoc here. A child <source> element can't bind to
> the tree while its parent is not in the tree.

Not true, we're getting called with a null document when a source element is appended to a <video> which isn't yet in a doc, so we need the check there. Here's a stack:

gklayout.dll!nsHTMLSourceElement::BindToTree(nsIDocument * aDocument=0x00000000, nsIContent * aParent=0x05868408, nsIContent * aBindingParent=0x00000000, int aCompileEventHandlers=0x00000001)  Line 146	C++
gklayout.dll!nsGenericElement::doInsertChildAt(nsIContent * aKid=0x05833338, unsigned int aIndex=0x00000000, int aNotify=0x00000001, nsIContent * aParent=0x05868408, nsIDocument * aDocument=0x00000000, nsAttrAndChildArray & aChildArray={...})  Line 3257 + 0x1c bytes	C++
gklayout.dll!nsGenericElement::InsertChildAt(nsIContent * aKid=0x05833338, unsigned int aIndex=0x00000000, int aNotify=0x00000001)  Line 3202 + 0x25 bytes	C++
gklayout.dll!nsGenericElement::doReplaceOrInsertBefore(int aReplace=0x00000000, nsIDOMNode * aNewChild=0x05833358, nsIDOMNode * aRefChild=0x00000000, nsIContent * aParent=0x05868408, nsIDocument * aDocument=0x00000000, nsIDOMNode * * aReturn=0x002fe6a8)  Line 3942 + 0x1c bytes	C++
gklayout.dll!nsGenericElement::InsertBefore(nsIDOMNode * aNewChild=0x05833358, nsIDOMNode * aRefChild=0x00000000, nsIDOMNode * * aReturn=0x002fe6a8)  Line 3524 + 0x20 bytes	C++
gklayout.dll!nsHTMLVideoElement::InsertBefore(nsIDOMNode * newChild=0x05833358, nsIDOMNode * refChild=0x00000000, nsIDOMNode * * _retval=0x002fe6a8)  Line 52 + 0x18 bytes	C++
gklayout.dll!nsGenericElement::AppendChild(nsIDOMNode * aNewChild=0x05833358, nsIDOMNode * * aReturn=0x002fe6a8)  Line 502	C++
gklayout.dll!nsHTMLVideoElement::AppendChild(nsIDOMNode * newChild=0x05833358, nsIDOMNode * * _retval=0x002fe6a8)  Line 52 + 0x14 bytes	C++
xpc3250.dll!nsIDOMNode_AppendChild(JSContext * cx=0x0400b080, unsigned int argc=0x00000001, int * vp=0x041b4318)  Line 2415 + 0x3e bytes	C++
js3250.dll!js_Interpret(JSContext * cx=0x0400b080)  Line 5006 + 0x17 bytes	C++
[...]


(In reply to comment #10)
> Actually a better idea is probably to add a bit to IsNodeOfType in nsINode.

You mean like this?

>  if (!aParent->IsNodeOfType(nsINode::eMEDIA))
>    return NS_OK;
>
>  nsHTMLMediaElement* media = reinterpret_cast<nsHTMLMediaElement*>(aParent);
OK, I lied about IsInDoc.

That's the right approach for IsNodeOfType, but you should use a static_cast instead of reinterpret_cast.
Attached patch Patch v3 (obsolete) — Splinter Review
As v2, with roc's review comments addressed. Based on top of patch for bug 476731.
Attachment #361206 - Attachment is obsolete: true
Attachment #361459 - Flags: superreview?(roc)
Attachment #361459 - Flags: review?(chris.double)
Attachment #361206 - Flags: superreview?(roc)
Attachment #361206 - Flags: review?(chris.double)
+nsHTMLMediaElement::IsNodeOfType(PRUint32 aFlags) const
+{
+  return !(aFlags & ~(eCONTENT | eELEMENT | eMEDIA));
+}

Should call the superclass method here.

Maybe it would be simpler if nsHTMLSourceElement::BindToTree just called a method nsHTMLMediaElement::NotifyAddedSource which does the check and fires the event if necessary.

nsHTMLMediaElement::DoneAddingChildren calls Load synchronously. Perhaps it should be firing the event instead.
(In reply to comment #14)
> +nsHTMLMediaElement::IsNodeOfType(PRUint32 aFlags) const
> +{
> +  return !(aFlags & ~(eCONTENT | eELEMENT | eMEDIA));
> +}
> 
> Should call the superclass method here.

Hmmm... No other implementation of IsNodeOfType() does. Do we want to diverge from the standard approach?

For it to be correct, it would have to be:

PRBool
nsHTMLMediaElement::IsNodeOfType(PRUint32 aFlags) const
{
  PRBool isGenericElement = nsGenericHTMLElement::IsNodeOfType(aFlags & ~eMEDIA);
  return isGenericElement ||
         isGenericElement && (aFlags & eMEDIA) ||
         aFlags == eMEDIA;
}

Surely the standard approach is cleaner? Oops, the original should return !(aFlags & ~(eCONTENT | eELEMENT | eHTML | eMEDIA)).
(In reply to comment #15)
> (In reply to comment #14)
> > +nsHTMLMediaElement::IsNodeOfType(PRUint32 aFlags) const
> > +{
> > +  return !(aFlags & ~(eCONTENT | eELEMENT | eMEDIA));
> > +}
> > 
> > Should call the superclass method here.
> 
> Hmmm... No other implementation of IsNodeOfType() does. Do we want to diverge
> from the standard approach?

No, although that standard approach is pretty bad, because it means if you want to add a flag say to HTML elements you have to find all the subclasses implementing IsNodeOfType and modify them. But let's do that anyway.
Attached patch Patch v4 (obsolete) — Splinter Review
As v3, but with nsHTMLMediaElement::NotifySourceAdded(), as per comment 14.
Attachment #361459 - Attachment is obsolete: true
Attachment #361640 - Flags: superreview?(roc)
Attachment #361640 - Flags: review?(chris.double)
Attachment #361459 - Flags: superreview?(roc)
Attachment #361459 - Flags: review?(chris.double)
MediaLoadEvent should move to nsHTMLMediaElement.cpp, it's not needed in the header file anymore.

Shouldn't we coalesce multiple pending load events? Or rather, have Load() clear out all pending load events, since we want an explicit load() call to cancel any pending events too?
(In reply to comment #18)
> Shouldn't we coalesce multiple pending load events? Or rather, have Load()
> clear out all pending load events, since we want an explicit load() call to
> cancel any pending events too?

That's not specified behaviour... Chris Double also raised this issue, and he's going to post to WHATWG to try and get some feedback on this.
For the record, we decided it actually is the specified behaviour.
Attached patch Patch v5 (obsolete) — Splinter Review
With load task coalesced, includes mochitest for that case.
Attachment #361640 - Attachment is obsolete: true
Attachment #361699 - Flags: superreview?(roc)
Attachment #361699 - Flags: review?(chris.double)
Attachment #361640 - Flags: superreview?(roc)
Attachment #361640 - Flags: review?(chris.double)
This isn't quite the right way to do it. Consider this example:
1) script appends <source> to trigger implicit load, sets mLoadTaskQueued to true
2) script calls load() explicitly, sets mLoadTaskQueued to false
3) script triggers some asynchronous event dispatch, say volumechanged
4) script appends another <source> to trigger another implicit load, sets 
mLoadTaskQueued to true
5) MediaLoadEvent from step #1 runs, does the load(), which fires "aborted" synchronously and sets mLoadTaskQueued to false
6) "volumechanged" handler runs

Basically the MediaLoadEvent from step #1 should not run, the MediaLoadEvent from step #4 should run. Script can observe the difference because the implicit load appears to happen out of order with other asynchronously dispatched events.

How about we fix this by creating a load object for each load, like we talked about for the new load algorithm. But here we don't have to move any functionality into the load object, we can just use it to check if events correspond to the current load and have them do nothing if they're not.
Attached patch Patch v6 (obsolete) — Splinter Review
Patch v6. As v5 expect:
* Adds nsMediaLoadObject to track loads/events.
* Modified test_load_coalescing.html to test case mentioned by Roc in comment 22.
* Changed load() so that it returns NS_OK on failure - failure codes are interpreted as exceptions in JS, and that's not supposed to happen, errors in load() are expressed by error events. This was required for test_load_coalescing.html to work.
Attachment #361699 - Attachment is obsolete: true
Attachment #361851 - Flags: superreview?(roc)
Attachment #361851 - Flags: review?(chris.double)
Attachment #361699 - Flags: superreview?(roc)
Attachment #361699 - Flags: review?(chris.double)
Attachment #361851 - Flags: superreview?(roc) → superreview+
Comment on attachment 361851 [details] [diff] [review]
Patch v6

Your patch will need to be updated to the trunk since I landed some changes last night.
This class shouldn't need to inherit from nsISupports just to get refcounting. We should use inline Addref/Release like Thebes does:
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/public/gfxTypes.h#89
But we don't want to use these Thebes-specific macros. Benjamin, I think those macros should move to nsISupportsImpl.h (and be given generic names), what do you think?
Comment on attachment 361851 [details] [diff] [review]
Patch v6

>+  // Set a new load object. This will cause implicit load events which were
>+  // enqueued before with a differnet load object to silently be cancelled.

Nit: "differnet" spelling
Attachment #361851 - Flags: review?(chris.double) → review+
If you're going to have generic macros, please do it right, as in bug 421127. nsISupportsImpl.h is definitely not a good place for macros that aren't related to implementing nsISupports at all.
OK, we'll just use nsISupports here and wait on 421127 to be resolved.
We could use nsCountedRef from nsAutoRef.h, but still we'd need to add something similar to THEBES_INLINE_DECL_REFCOUNTING() in nsAutoRef.h, as nsAutoRef.h doesn't add a refcounting implementation, just a pointer to objects that are refcounted.
Attached patch Patch v7 (obsolete) — Splinter Review
As v6, but updated to tip, with spelling error fixed. Still using nsISupports for nsMediaLoad. r+ doublec, sr+ roc.
Attachment #361851 - Attachment is obsolete: true
Attachment #362169 - Flags: superreview+
Attachment #362169 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Note this patch is based on bug 476731, that must land before this one can.
Keywords: checkin-needed
Whiteboard: [needs landing]
Keywords: checkin-needed
Whiteboard: [needs landing]
Keywords: checkin-needed
Whiteboard: [needs landing]
I re-checked out my tree and forgot to re-add the test files, here's the patch v7 with tests re-added. Still applies cleanly to trunk.
Attachment #362169 - Attachment is obsolete: true
Attachment #362491 - Flags: superreview+
Attachment #362491 - Flags: review+
Pushed http://hg.mozilla.org/mozilla-central/rev/599deb701590
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Whiteboard: [needs 191 landing] → [needs 191 landing] [tb3needs]
You need to log in before you can comment on or make changes to this bug.