Open
Bug 886194
Opened 12 years ago
Updated 2 years ago
Add MediaSource support to HTMLMediaElement.srcObject
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
NEW
People
(Reporter: kinetik, Assigned: alwu)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed, Whiteboard: dev-doc-needed)
Attachments
(1 file, 6 obsolete files)
15.70 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
Updated•11 years ago
|
Assignee: nobody → jyavenard
Comment 1•11 years ago
|
||
Bug 886194 - Add MediaSource support to HTMLMediaElement.mozSrcObject
Attachment #8465370 -
Flags: review?(kinetik)
Comment 2•11 years ago
|
||
Bug 886194 - Add MediaSource support to HTMLMediaElement.mozSrcObject
Attachment #8465379 -
Flags: review?(kinetik)
Updated•11 years ago
|
Attachment #8465370 -
Attachment is obsolete: true
Attachment #8465370 -
Flags: review?(kinetik)
Comment 3•11 years ago
|
||
added a little test there:
http://people.mozilla.org/~jyavenard/simple-dash/
variable "mediaobj" contains the MediaSource object
Reporter | ||
Comment 4•11 years ago
|
||
Comment on attachment 8465379 [details] [diff] [review]
Add MediaSource support to HTMLMediaElement.mozSrcObject
Review of attachment 8465379 [details] [diff] [review]:
-----------------------------------------------------------------
Punting to roc since this is his feature request/proposal.
Attachment #8465379 -
Flags: review?(kinetik) → review?(roc)
Comment on attachment 8465379 [details] [diff] [review]
Add MediaSource support to HTMLMediaElement.mozSrcObject
Review of attachment 8465379 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/html/content/src/HTMLMediaElement.cpp
@@ +530,5 @@
> {
> + if (aValue.Value().IsMediaStream()) {
> + mSrcAttrStream = &(aValue.Value().GetAsMediaStream());
> + }
> + else if (aValue.Value().IsMediaSource()) {
} else if {
@@ +532,5 @@
> + mSrcAttrStream = &(aValue.Value().GetAsMediaStream());
> + }
> + else if (aValue.Value().IsMediaSource()) {
> + mMediaSource = &(aValue.Value().GetAsMediaSource());
> + }
I think we need to clear mMediaSource and/or mSrcAttrStream here.
Attachment #8465379 -
Flags: review?(roc) → review-
Comment 6•11 years ago
|
||
Bug 886194 - Add MediaSource support to HTMLMediaElement.mozSrcObject
Attachment #8467702 -
Flags: review?(roc)
Updated•11 years ago
|
Attachment #8465379 -
Attachment is obsolete: true
Comment 7•11 years ago
|
||
http://people.mozilla.org/~jyavenard/simple-dash/ now setup and instantiate MediaSource by setting the mozSrcObject attribute
Comment on attachment 8467702 [details] [diff] [review]
Add MediaSource support to HTMLMediaElement.mozSrcObject
Review of attachment 8467702 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/html/content/src/HTMLMediaElement.cpp
@@ +505,5 @@
>
> +void
> +HTMLMediaElement::GetMozSrcObject(Nullable<OwningMediaStreamOrMediaSource>& aValue) const
> +{
> + if (mSrcAttrStream && mSrcAttrStream->GetStream()) {
You should remove this GetStream check unless you have a good reason for adding it. You should probably keep it as an assertion like the old code had.
@@ +532,3 @@
> {
> + if (aValue.IsNull()) {
> + return;
If it's null, then we need to clear both mSrcAttrMediaSource and mSrcAttrStream, and we should also call Load().
The code's probably simpler if we clear them both first before we check aValue.
@@ +863,5 @@
> nsAutoString src;
> if (mSrcAttrStream) {
> SetupSrcMediaStreamPlayback(mSrcAttrStream);
> + } else if (mSrcAttrMediaSource) {
> + nsresult rv = LoadResource();
I don't think we want to go into LoadResource() here. LoadResource() sets up Necko stuff to start reading data from an nsIChannel. You don't want to do that. Instead I think we should follow what we did for mSrcAttrStream: factor out the MediaSource setup code into a helper method, say SetupSrcMediaSourcePlayback, and call it from here and from LoadResource when LoadResource encounters a MediaSource URI.
@@ +1205,5 @@
> const char16_t* params[] = { spec.get() };
> ReportLoadError("MediaLoadInvalidURI", params, ArrayLength(params));
> return rv;
> }
> + return LoadResource(source);
Right, LoadResource is that helper method, but it should have a different name and you should call it directly from SelectResource for mSrcAttrMediaSources instead of coming through here.
@@ +2948,5 @@
> ChangeReadyState(nsIDOMHTMLMediaElement::HAVE_METADATA);
> DispatchAsyncEvent(NS_LITERAL_STRING("durationchange"));
> DispatchAsyncEvent(NS_LITERAL_STRING("loadedmetadata"));
> + if (!mMediaSource &&
> + mDecoder && mDecoder->IsTransportSeekable() && mDecoder->IsMediaSeekable()) {
This should be if (mLoadingSrc && ...). No need to check mMediaSource here.
Attachment #8467702 -
Flags: review?(roc) → review-
Comment 9•11 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> Comment on attachment 8467702 [details] [diff] [review]
> Add MediaSource support to HTMLMediaElement.mozSrcObject
>
> Review of attachment 8467702 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: content/html/content/src/HTMLMediaElement.cpp
> @@ +505,5 @@
> >
> > +void
> > +HTMLMediaElement::GetMozSrcObject(Nullable<OwningMediaStreamOrMediaSource>& aValue) const
> > +{
> > + if (mSrcAttrStream && mSrcAttrStream->GetStream()) {
>
> You should remove this GetStream check unless you have a good reason for
> adding it. You should probably keep it as an assertion like the old code had.
Actually it should have been && !mSrcAttrStream->GetStream().
>
> @@ +532,3 @@
> > {
> > + if (aValue.IsNull()) {
> > + return;
>
> If it's null, then we need to clear both mSrcAttrMediaSource and
> mSrcAttrStream, and we should also call Load().
>
> The code's probably simpler if we clear them both first before we check
> aValue.
fair enough...
>
> @@ +863,5 @@
> > nsAutoString src;
> > if (mSrcAttrStream) {
> > SetupSrcMediaStreamPlayback(mSrcAttrStream);
> > + } else if (mSrcAttrMediaSource) {
> > + nsresult rv = LoadResource();
>
> I don't think we want to go into LoadResource() here. LoadResource() sets up
> Necko stuff to start reading data from an nsIChannel. You don't want to do
> that. Instead I think we should follow what we did for mSrcAttrStream:
> factor out the MediaSource setup code into a helper method, say
> SetupSrcMediaSourcePlayback, and call it from here and from LoadResource
> when LoadResource encounters a MediaSource URI.
I tried to keep the logic as close as possible to the original code.
LoadResource() exits early should mSrcAttrMediaSource be set.
But not before additional tests are dong on mChannel and testing if "media is allowed for the docshell".
Not knowing the history nor utility of those extra checks I made sure the code still went through them...
If you're saying that those aren't of used for mediasource, I will remove it, will also make for simpler code.
> @@ +2948,5 @@
> > ChangeReadyState(nsIDOMHTMLMediaElement::HAVE_METADATA);
> > DispatchAsyncEvent(NS_LITERAL_STRING("durationchange"));
> > DispatchAsyncEvent(NS_LITERAL_STRING("loadedmetadata"));
> > + if (!mMediaSource &&
> > + mDecoder && mDecoder->IsTransportSeekable() && mDecoder->IsMediaSeekable()) {
>
> This should be if (mLoadingSrc && ...). No need to check mMediaSource here.
It is possible to use MediaSource and have mLoadingSrc set, should the src attribute been set instead.
If we were to check for mLoadingSrc, when using MediaSource it would continue to attempt to call ProcessMediaFragmentURI which makes no sense with MediaSource, just wasting cycles...
Hence, why I tested if mMediaSource was set instead of testing mLoadingSrc: it's slightly better optimised.
Let me know if you think otherwise
Comment 10•11 years ago
|
||
Bug 886194 - Add MediaSource support to HTMLMediaElement.mozSrcObject
Attachment #8468306 -
Flags: review?(roc)
Updated•11 years ago
|
Attachment #8467702 -
Attachment is obsolete: true
Comment on attachment 8468306 [details] [diff] [review]
Add MediaSource support to HTMLMediaElement.mozSrcObject
Review of attachment 8468306 [details] [diff] [review]:
-----------------------------------------------------------------
Basically looks good. Just one little thing to fix.
It needs an automated test. That can be a separate patch or rolled into this one.
::: content/html/content/src/HTMLMediaElement.cpp
@@ +2886,5 @@
> + return NS_ERROR_FAILURE;
> + }
> + mMediaSource = aSource;
> + if (aSource != mSrcAttrMediaSource) {
> + mSrcAttrMediaSource = aSource;
Don't do this. If you do this, then that means someone can set .src to a MediaSource URL, then reading srcObject will get the underlying MediaSource object. I'm pretty sure the spec says not to do that. At least, that's not how MediaStreams work.
Attachment #8468306 -
Flags: review?(roc) → review-
Comment 12•11 years ago
|
||
Bug 886194 - Carrying r+
Updated•11 years ago
|
Attachment #8468306 -
Attachment is obsolete: true
Updated•11 years ago
|
Keywords: dev-doc-needed
Blocks: 1062045
Merged 201937: Bug 1057173 - Rename MediaKeyNeededEvent to MediaEncryptedEvent.
Updated•10 years ago
|
Attachment #8469679 -
Attachment is obsolete: true
Comment 14•10 years ago
|
||
Update commit message
Updated•10 years ago
|
Attachment #8483221 -
Attachment is obsolete: true
Comment 15•10 years ago
|
||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 16•10 years ago
|
||
this needs a dom peer review:
remote: WebIDL file dom/webidl/HTMLMediaElement.webidl altered in changeset 8f1c3a97e8ac without DOM peer review
Keywords: checkin-needed
Comment 17•10 years ago
|
||
Those B2G M7 failures on the Try run are scary-looking too. Are you 100% certain that they aren't from your patch?
Comment on attachment 8485565 [details] [diff] [review]
Add MediaSource support to HTMLMediaElement.mozSrcObject
Review of attachment 8485565 [details] [diff] [review]:
-----------------------------------------------------------------
Needs DOM peer review
Attachment #8485565 -
Flags: review?(bugs)
Comment 19•10 years ago
|
||
Comment on attachment 8485565 [details] [diff] [review]
Add MediaSource support to HTMLMediaElement.mozSrcObject
We really must get rid of mozSrcObject at some point, but since this isn't adding that property, fine.
Do we still have moz prefix because we don't implement the Blob part of srcObject?
Attachment #8485565 -
Flags: review?(bugs) → review+
We have the moz prefix because it wasn't specced and it was added before we changed our policy. Now that it is specced, we should remove the prefix, but I guess we should have an overlap where we support both since there's content out there using mozSrcObject.
Comment 21•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> Those B2G M7 failures on the Try run are scary-looking too. Are you 100%
> certain that they aren't from your patch?
not 100%, but fairly close.
Re-running those tests and they ran fine.
Updated•10 years ago
|
Keywords: checkin-needed
Comment 22•10 years ago
|
||
Keywords: checkin-needed
Comment 23•10 years ago
|
||
sorry had to backout this change since it seems this caused various test failures like:
https://tbpl.mozilla.org/php/getParsedLog.php?id=47951379&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=47950605&tree=Mozilla-Inbound
Comment 24•10 years ago
|
||
that is very weird !
why wouldn't these show on try?
Comment 25•10 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #24)
> that is very weird !
>
> why wouldn't these show on try?
seems you run the try on opt builds while the failures were on debug builds
Updated•10 years ago
|
Flags: needinfo?(jyavenard)
Priority: -- → P5
Updated•9 years ago
|
Component: Audio/Video → Audio/Video: Playback
Updated•8 years ago
|
Summary: Add MediaSource support to HTMLMediaElement.mozSrcObject → Add MediaSource support to HTMLMediaElement.srcObject
Assignee | ||
Comment 27•6 years ago
|
||
Do we have any particular reason not to reland this bug? If not, I can help to rebase and reland this one.
Flags: needinfo?(jyavenard)
Comment 28•6 years ago
|
||
(In reply to Alastor Wu [:alwu] from comment #27)
Do we have any particular reason not to reland this bug? If not, I can help to rebase and reland this one.
this was the first bug I ever worked on, the logic was probably wrong :)
but sure, we can still fix that bug, probably wouldn’t do it the same way today.
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 29•6 years ago
|
||
Okay, will continue work on this one later.
Assignee: jyavenard → alwu
Priority: P5 → P3
Whiteboard: dev-doc-needed
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•