Closed Bug 792665 Opened 12 years ago Closed 12 years ago

Separate HTMLMediaElement.src from HTMLMediaElement.srcObject

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox17 --- fixed

People

(Reporter: roc, Assigned: roc)

Details

(Keywords: dev-doc-complete, Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

      No description provided.
Maybe we should put .srcObject behind a pref?
Or else we should prefix it.
I suppose prefix it, though the entire way we (all the browsers) handle not-yet-agreed-to specs makes me want to pull my hair out....

I'm tempted to use what Google is in PeerConnection: add a suffix that gets bumped whenever the API changes, so if two browsers implement the same API, you're good.  It also handles the case of our first implementation is replaced with a newer-but-not-final one.

So that would be video.srcObject01 = stream;
That style is relatively unknown and I fear would create much confusion. I'll go with prefixing for now.

Anant, Mike: I'm worried that this change might break some B2G apps. OTOH it'll be good to have this API aligned with standards before B2G apps get locked down. Can you notify whoever needs to know that it's happening?
Attached patch fix (obsolete) — Splinter Review
Attachment #662814 - Flags: review?(cpearce)
Comment on attachment 662814 [details] [diff] [review]
fix

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

Also, we'll need URL.createObjectURL(stream) and support for handling those URLs in video.src.  (This can be a separate bug)

r+ if you're happy with your responses to my questions.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +463,5 @@
>      }
>    }
> +  // Should we store unsupported values on the element's attribute anyway?
> +  // Let's not.
> +  return NS_OK;

What if you do:

video.src = foo;
video.srcObject = stream;
video.srcObject = null;

Should it stop playing?  (I think so; is null an object here?).

Should it restart src? (Good question.  Currently I don't think it does, but what happens if you fiddle controls?)

Similar questions for the other way around.
Attachment #662814 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #6)
> Also, we'll need URL.createObjectURL(stream) and support for handling those
> URLs in video.src.  (This can be a separate bug)

Yes, I'm working on that in bug 792675.

At some stage we should add support for element.srcObject = blob as well.

> What if you do:
> 
> video.src = foo;
> video.srcObject = stream;
> video.srcObject = null;
> 
> Should it stop playing?  (I think so; is null an object here?).

Yes, it should accept null and stop playing the stream (and start playing foo again). I need to fix that, right now setting to null would just be ignored. If you do these three statements all at once then the overall effect would be the same as video.src = foo --- restarting playback of foo.

> Similar questions for the other way around.

You mean 'video.srcObject = stream; video.src = foo; video.src = ""'? That's less interesting; srcObject always wins, so the assignments to src don't change anything. We would restart playback of 'stream', but that's basically a no-op anyway.
Comment on attachment 662814 [details] [diff] [review]
fix

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

::: content/media/test/Makefile.in
@@ +129,5 @@
>  		test_seekLies.html \
>  		test_media_sniffer.html \
>  		contentType.sjs \
> +		test_streams_srcObject.html \
> +		test_streams_element_capture_bug752784.html \

test_streams_element_capture_bug752784.html isn't included in the patch?
Attachment #662814 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #8)
> test_streams_element_capture_bug752784.html isn't included in the patch?

It's already in the tree but someone forgot to add it Makefile.in.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> (In reply to Chris Pearce (:cpearce) from comment #8)
> > test_streams_element_capture_bug752784.html isn't included in the patch?
> 
> It's already in the tree but someone forgot to add it Makefile.in.

Actually it's not. It's only in my tree and we already added that test as a crashtest elsewhere. So I removed that line.
Attached patch updated patchSplinter Review
Fixed the issues in comment #7. This is ready for landing.
Attachment #662814 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/3425c7e124b0
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment on attachment 663186 [details] [diff] [review]
updated patch

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

This is an API change. We should take it on Aurora before the uplift to beta, to head off people trying to use the old API. Very few people are using it yet so I don't expect this to cause any compatibility issues right now, but we don't want any to develop.
Attachment #663186 - Flags: approval-mozilla-aurora?
Attachment #663186 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
I was trying to document this but wasn't able to fully understand the change here.

I see in the MDN doc that .src is already correctly documented (regarding its behavior in HTML5). By looking at the tests in the patch here I see that .srcObject deals with streams, and that this is a different spec (AFAIK there is no notion of media streams in the HTML5 spec). 

I'm a little bit lost with all the "streams" I see. There's is roc's proposal [1] but also those I found in WebRTC [2]. Are they the same or, if not, which one are we talking about for .srcObject?


[1] https://dvcs.w3.org/hg/audio/raw-file/tip/streams/StreamProcessing.html
[2] https://developer.mozilla.org/en-US/docs/WebRTC/MediaStream_API
They are the same. For now, #1 is not on standards track, so feel free to refer to #2. Thanks a ton!
You need to log in before you can comment on or make changes to this bug.