Matthew Gregan [:kinetik] Reporter
	Description • 11 years ago

See bug 855130 comment 47.

	Jean-Yves Avenard [:jya]
	Comment 1 • 10 years ago

Bug 886194 - Add MediaSource support to HTMLMediaElement.mozSrcObject

	Jean-Yves Avenard [:jya]
	Comment 2 • 10 years ago

Bug 886194 - Add MediaSource support to HTMLMediaElement.mozSrcObject

	Jean-Yves Avenard [:jya]
	Comment 3 • 10 years ago

added a little test there:
http://people.mozilla.org/~jyavenard/simple-dash/

variable "mediaobj" contains the MediaSource object

	Matthew Gregan [:kinetik] Reporter
	Comment 4 • 10 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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 5 • 10 years ago

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.

	Jean-Yves Avenard [:jya]
	Comment 6 • 10 years ago

Bug 886194 - Add MediaSource support to HTMLMediaElement.mozSrcObject

	Jean-Yves Avenard [:jya]
	Comment 7 • 10 years ago

http://people.mozilla.org/~jyavenard/simple-dash/ now setup and instantiate MediaSource by setting the mozSrcObject attribute

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 10 years ago

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.

	Jean-Yves Avenard [:jya]
	Comment 9 • 10 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

	Jean-Yves Avenard [:jya]
	Comment 10 • 10 years ago

Bug 886194 - Add MediaSource support to HTMLMediaElement.mozSrcObject

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 11 • 10 years ago

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.

	Jean-Yves Avenard [:jya]
	Comment 12 • 10 years ago

Bug 886194 - Carrying r+

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 13 • 10 years ago

Merged 201937: Bug 1057173 - Rename MediaKeyNeededEvent to MediaEncryptedEvent.

	Jean-Yves Avenard [:jya]
	Comment 14 • 10 years ago

Update commit message

	Jean-Yves Avenard [:jya]
	Comment 15 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=b1a632f24414

	Carsten Book [:Tomcat]
	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

	Ryan VanderMeulen [:RyanVM]
	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?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 18 • 10 years ago

Comment on attachment 8485565 [details] [diff] [review]
Add MediaSource support to HTMLMediaElement.mozSrcObject

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

Needs DOM peer review

	Olli Pettay [:smaug][bugs@pettay.fi]
	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?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 20 • 10 years ago

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.

	Jean-Yves Avenard [:jya]
	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.

	Carsten Book [:Tomcat]
	Comment 22 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/408273e953e9

	Carsten Book [:Tomcat]
	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

	Jean-Yves Avenard [:jya]
	Comment 24 • 10 years ago

that is very weird !

why wouldn't these show on try?

	Carsten Book [:Tomcat]
	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

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)
	Comment 26 • 10 years ago

Need to reland