Add MediaSource support to HTMLMediaElement.srcObject

NEW
Assigned to

Status

()

P3
normal
6 years ago
a month ago

People

(Reporter: kinetik, Assigned: alwu)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: dev-doc-needed)

Attachments

(1 attachment, 6 obsolete attachments)

Assignee: nobody → jyavenard
Bug 886194 - Add MediaSource support to HTMLMediaElement.mozSrcObject
Attachment #8465370 - Flags: review?(kinetik)
Bug 886194 - Add MediaSource support to HTMLMediaElement.mozSrcObject
Attachment #8465379 - Flags: review?(kinetik)
Attachment #8465370 - Attachment is obsolete: true
Attachment #8465370 - Flags: review?(kinetik)
added a little test there:
http://people.mozilla.org/~jyavenard/simple-dash/

variable "mediaobj" contains the MediaSource object
(Reporter)

Comment 4

5 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-
Bug 886194 - Add MediaSource support to HTMLMediaElement.mozSrcObject
Attachment #8467702 - Flags: review?(roc)
Attachment #8465379 - Attachment is obsolete: true
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-
(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
Bug 886194 - Add MediaSource support to HTMLMediaElement.mozSrcObject
Attachment #8468306 - Flags: review?(roc)
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-
Attachment #8468306 - Attachment is obsolete: true
Keywords: dev-doc-needed
Merged 201937: Bug 1057173 - Rename MediaKeyNeededEvent to MediaEncryptedEvent.
Attachment #8469679 - Attachment is obsolete: true
Attachment #8483221 - Attachment is obsolete: true
Keywords: checkin-needed
this needs a dom peer review:

remote: WebIDL file dom/webidl/HTMLMediaElement.webidl altered in changeset 8f1c3a97e8ac without DOM peer review
Keywords: checkin-needed
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 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.
(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.
Keywords: checkin-needed
that is very weird !

why wouldn't these show on try?
(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
Need to reland
Flags: needinfo?(jyavenard)
Flags: needinfo?(jyavenard)
Priority: -- → P5
Component: Audio/Video → Audio/Video: Playback
Summary: Add MediaSource support to HTMLMediaElement.mozSrcObject → Add MediaSource support to HTMLMediaElement.srcObject
(Assignee)

Comment 27

2 months 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)

(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

a month ago

Okay, will continue work on this one later.

Assignee: jyavenard → alwu
Priority: P5 → P3
Whiteboard: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.