Re: Add srcObject attribute of type MediaStream to HTMLMediaElement. (issue 1815033003 by guidou@chromium.org)
533 views
Skip to first unread message
gui...@chromium.org
Mar 18, 2016, 10:17:17 AM3/18/16
to har...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, phili...@opera.com, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org
Reviewers: haraken
CL: https://codereview.chromium.org/1815033003/
Message:
Hi, PTAL
Description:
Add srcObject attribute of type MediaStream to HTMLMediaElement.
Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/D20_5X-2nCk
BUG=387740
Base URL: https://chromium.googlesource.com/chromium/src.git@master
Affected files (+260, -50 lines):
A third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
M third_party/WebKit/LayoutTests/webexposed/element-instance-property-listing-expected.txt
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
M third_party/WebKit/Source/core/html/HTMLMediaElement.h
M third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
A third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.h
A third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
A third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl
M third_party/WebKit/Source/modules/modules.gypi
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
CL: https://codereview.chromium.org/1815033003/
Message:
Hi, PTAL
Description:
Add srcObject attribute of type MediaStream to HTMLMediaElement.
Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/D20_5X-2nCk
BUG=387740
Base URL: https://chromium.googlesource.com/chromium/src.git@master
Affected files (+260, -50 lines):
A third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
M third_party/WebKit/LayoutTests/webexposed/element-instance-property-listing-expected.txt
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
M third_party/WebKit/Source/core/html/HTMLMediaElement.h
M third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
A third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.h
A third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
A third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl
M third_party/WebKit/Source/modules/modules.gypi
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in
har...@chromium.org
Mar 18, 2016, 7:37:41 PM3/18/16
to gui...@chromium.org, philipj+...@opera.com, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
(right):
https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp#newcode1
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
2016
https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.h
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.h
(right):
https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.h#newcode30
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.h:30:
PersistentWillBeMember<MediaStream> m_srcObject;
Are you sure that the Persistent handle doesn't cause a leak?
- HTMLMediaElement and HTMLMediaElementMediaStream have the same
lifetime.
- HTMLMediaElementMediaStream has a persistent to MediaStream.
- If the MediaStream has a strong reference to the DOM tree that
contains the HTMLMediaElement, there is a cycle.
(This issue will be gone once we remove the WillBe types, which will
happen soon.)
https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl
(right):
https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl#newcode9
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl:9:
] partial interface HTMLMediaElement {
Would you add:
// TODO(haraken): Per the spec, the srcObject should be defined in
HTMLMediaElement.idl. However, we cannot define srcObject in
HTMLMediaElement.idl because of the dependency restriction from modules/
to core/. For now we avoid the problem by using a partial interface.
https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl#newcode10
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl:10:
attribute MediaStream? srcObject;
The spec says that the type is MediaProvider?. Is it okay to use
MediaStream? ?
https://codereview.chromium.org/1815033003/
gui...@chromium.org
Mar 20, 2016, 8:16:30 AM3/20/16
to har...@chromium.org, philipj+...@opera.com, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
(right):
https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp#newcode1
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/03/18 23:37:40, haraken wrote:
>
> 2016
Done.
>
> 2016
Done.
https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.h
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.h
(right):
https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.h#newcode30
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.h:30:
PersistentWillBeMember<MediaStream> m_srcObject;
On 2016/03/18 23:37:40, haraken wrote:
>
> Are you sure that the Persistent handle doesn't cause a leak?
>
> - HTMLMediaElement and HTMLMediaElementMediaStream have the same
lifetime.
> - HTMLMediaElementMediaStream has a persistent to MediaStream.
> - If the MediaStream has a strong reference to the DOM tree that
contains the
> HTMLMediaElement, there is a cycle.
>
> (This issue will be gone once we remove the WillBe types, which will
happen
> soon.)
>
> Are you sure that the Persistent handle doesn't cause a leak?
>
> - HTMLMediaElement and HTMLMediaElementMediaStream have the same
lifetime.
> - HTMLMediaElementMediaStream has a persistent to MediaStream.
> - If the MediaStream has a strong reference to the DOM tree that
contains the
> HTMLMediaElement, there is a cycle.
>
> (This issue will be gone once we remove the WillBe types, which will
happen
> soon.)
I ran a few experiments and observed that the lifecycles of the
HTMLMediaElement and the MediaStream objects were the same with
assigment using the srcObject attribute as with assigment via the src
attribute and URL.createObjectURL. Note that with the src attribute
there is no Persistent reference. Thus, I think the Persistent reference
here changes nothing, given how things work today. This was true with
and without Oilpan.
However, that doesn't mean things are perfect.
For example, once a MediaStream is assigned to an element, it is
registered in MediaStreamRegistry, which has a persistent reference to
the MediaStream. The MediaStreams are GC'ed only when the registry is
stopped.
There is also some reference from MediaStream to the DOM tree, although
I don't know exactly where it is. This is a brief summary of my
experiments:
Experiment 1:
var v = document.createElement('video')
var ms = null
navigator.getUserMedia({video:true}, stream=>{ms=stream}, _=>{})
v = null;
gc(); // video is released
ms = null;
gc(); // mediastream is released
Experiment 2:
var v = document.createElement('video')
navigator.getUserMedia({video:true}, stream=>{v.srcObject=stream},
_=>{})
v.srcObject = null
gc(); //nothing is released
v = null
gc(); //video is released
// Reload page
gc(); //mediastream is released
Experiment 3:
var v = document.createElement('video')
navigator.getUserMedia({video:true}, stream=>{v.srcObject=stream},
_=>{})
v = null
gc(); //nothing is released
// Reload document
gc(); // video and mediastream are released
Like I said, the experiments above work the same for both srcObject and
src assigment, with and without oilpan.
My conclusion is that we should investigate this further.
I don't think this should block this CL, although it might be possible
that after we improve other things this Persistent reference might make
a difference. WDYT?
HTMLMediaElement and the MediaStream objects were the same with
assigment using the srcObject attribute as with assigment via the src
attribute and URL.createObjectURL. Note that with the src attribute
there is no Persistent reference. Thus, I think the Persistent reference
here changes nothing, given how things work today. This was true with
and without Oilpan.
However, that doesn't mean things are perfect.
For example, once a MediaStream is assigned to an element, it is
registered in MediaStreamRegistry, which has a persistent reference to
the MediaStream. The MediaStreams are GC'ed only when the registry is
stopped.
There is also some reference from MediaStream to the DOM tree, although
I don't know exactly where it is. This is a brief summary of my
experiments:
Experiment 1:
var v = document.createElement('video')
var ms = null
navigator.getUserMedia({video:true}, stream=>{ms=stream}, _=>{})
v = null;
gc(); // video is released
ms = null;
gc(); // mediastream is released
Experiment 2:
var v = document.createElement('video')
navigator.getUserMedia({video:true}, stream=>{v.srcObject=stream},
_=>{})
v.srcObject = null
gc(); //nothing is released
v = null
gc(); //video is released
// Reload page
gc(); //mediastream is released
Experiment 3:
var v = document.createElement('video')
navigator.getUserMedia({video:true}, stream=>{v.srcObject=stream},
_=>{})
v = null
gc(); //nothing is released
// Reload document
gc(); // video and mediastream are released
Like I said, the experiments above work the same for both srcObject and
src assigment, with and without oilpan.
My conclusion is that we should investigate this further.
I don't think this should block this CL, although it might be possible
that after we improve other things this Persistent reference might make
a difference. WDYT?
https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl
(right):
https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl#newcode9
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl:9:
] partial interface HTMLMediaElement {
On 2016/03/18 23:37:40, haraken wrote:
>
> Would you add:
>
> // TODO(haraken): Per the spec, the srcObject should be defined in
> HTMLMediaElement.idl. However, we cannot define srcObject in
> HTMLMediaElement.idl because of the dependency restriction from
modules/ to
> core/. For now we avoid the problem by using a partial interface.
>
> Would you add:
>
> // TODO(haraken): Per the spec, the srcObject should be defined in
> HTMLMediaElement.idl. However, we cannot define srcObject in
> HTMLMediaElement.idl because of the dependency restriction from
modules/ to
> core/. For now we avoid the problem by using a partial interface.
Done.
https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl#newcode10
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl:10:
attribute MediaStream? srcObject;
On 2016/03/18 23:37:40, haraken wrote:
>
> The spec says that the type is MediaProvider?. Is it okay to use
MediaStream? ?
>
> The spec says that the type is MediaProvider?. Is it okay to use
MediaStream? ?
Now using MediaProvider, which is typedef'ed to be MediaStream for now.
According to the spec, it can be MediaStream, MediaSource or Blob.
For now we support only MediaStream, but added a TODO to support the
other types mentioned in the spec.
As I explain in the I2I, supporting MediaStream puts us at the same
level of support of all the other major browsers.
https://codereview.chromium.org/1815033003/
According to the spec, it can be MediaStream, MediaSource or Blob.
For now we support only MediaStream, but added a TODO to support the
other types mentioned in the spec.
As I explain in the I2I, supporting MediaStream puts us at the same
level of support of all the other major browsers.
https://codereview.chromium.org/1815033003/
phi...@opera.com
Mar 21, 2016, 7:13:13 AM3/21/16
to gui...@chromium.org, har...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.h
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.h#newcode111
third_party/WebKit/Source/core/html/HTMLMediaElement.h:111: void
setSrcObjectURL(const AtomicString&);
Rather than using an object URL, can you look into something like
Source/platform/MediaProvider which wraps the MediaStream (and later
other types), with the isMediaStream()/getAsMediaStream() pattern that
generated union types have?
This class (maybe it has to be public/platform/WebMediaProvider) would
be used by FrameLoaderClientImpl::createWebMediaPlayer or
RenderFrameImpl::createMediaPlayer to create a WebMediaPlayerMS instance
for the MediaStream object.
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
(right):
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp#newcode29
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp:29:
AtomicString
url(URLMediaStream::createObjectURL(element.getExecutionContext(),
mediaStream));
If this URL is never revoked, won't the mediaStream object simply leak?
Getting rid of this problem is one of the main benefits of srcObject
over createObjectURL, if it can be fixed.
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl
(right):
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl#newcode12
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl:12:
] partial interface HTMLMediaElement {
As stated on blink-dev, I think this could be put in modules/srcobject,
which would be allowed to depend on modules/mediastream (now) and
modules/mediasource (later). Can you check if it's just a simple
renaming?
https://codereview.chromium.org/1815033003/
gui...@chromium.org
Mar 21, 2016, 12:34:54 PM3/21/16
to har...@chromium.org, philipj+...@opera.com, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.h
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.h#newcode111
third_party/WebKit/Source/core/html/HTMLMediaElement.h:111: void
setSrcObjectURL(const AtomicString&);
On 2016/03/21 11:13:13, philipj_UTC7 wrote:
> Rather than using an object URL, can you look into something like
> Source/platform/MediaProvider which wraps the MediaStream (and later
other
> types), with the isMediaStream()/getAsMediaStream() pattern that
generated union
> types have?
>
> This class (maybe it has to be public/platform/WebMediaProvider) would
be used
> by FrameLoaderClientImpl::createWebMediaPlayer or
> RenderFrameImpl::createMediaPlayer to create a WebMediaPlayerMS
instance for the
> MediaStream object.
> Rather than using an object URL, can you look into something like
> Source/platform/MediaProvider which wraps the MediaStream (and later
other
> types), with the isMediaStream()/getAsMediaStream() pattern that
generated union
> types have?
>
> This class (maybe it has to be public/platform/WebMediaProvider) would
be used
> by FrameLoaderClientImpl::createWebMediaPlayer or
> RenderFrameImpl::createMediaPlayer to create a WebMediaPlayerMS
instance for the
> MediaStream object.
Source/platform and public/platform cannot access MediaStream due to
dependency restrictions. Fortunately, this dependency problem has
already been solved for MediaStreams with the MediaStreamDescriptor
class in Source/platform.
The main difficulty I see of not using the URL is that all the code from
HTMLMediaElement to the creation of the WebMediaPlayer uses the URL, and
it's in the lowest layer where the MediaStreamDescriptor is obtained
from the URL.
I think we would need alternative non-URL versions of at least
HTMLMediaElement::loadResource(), HTMLMediaElement::startPlayerLoad(),
FrameLoaderClientImpl::createWebMediaPlayer(), and
RenderFrameImpl::createMediaPlayer(). At first sight, it looks like a
lot of work. Of course, I'm not familiar with HTMLMediaElement code, so
maybe there is a way to do it without duplicating much code.
dependency restrictions. Fortunately, this dependency problem has
already been solved for MediaStreams with the MediaStreamDescriptor
class in Source/platform.
The main difficulty I see of not using the URL is that all the code from
HTMLMediaElement to the creation of the WebMediaPlayer uses the URL, and
it's in the lowest layer where the MediaStreamDescriptor is obtained
from the URL.
I think we would need alternative non-URL versions of at least
HTMLMediaElement::loadResource(), HTMLMediaElement::startPlayerLoad(),
FrameLoaderClientImpl::createWebMediaPlayer(), and
RenderFrameImpl::createMediaPlayer(). At first sight, it looks like a
lot of work. Of course, I'm not familiar with HTMLMediaElement code, so
maybe there is a way to do it without duplicating much code.
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
(right):
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp#newcode29
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp:29:
AtomicString
url(URLMediaStream::createObjectURL(element.getExecutionContext(),
mediaStream));
On 2016/03/21 11:13:13, philipj_UTC7 wrote:
> If this URL is never revoked, won't the mediaStream object simply
leak? Getting
> rid of this problem is one of the main benefits of srcObject over
> createObjectURL, if it can be fixed.
> If this URL is never revoked, won't the mediaStream object simply
leak? Getting
> rid of this problem is one of the main benefits of srcObject over
> createObjectURL, if it can be fixed.
What if we do revoke the URL? For example, when srcObject becomes null
and perhaps in other cases?
and perhaps in other cases?
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl
(right):
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl#newcode12
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl:12:
] partial interface HTMLMediaElement {
On 2016/03/21 11:13:13, philipj_UTC7 wrote:
> As stated on blink-dev, I think this could be put in
modules/srcobject, which
> would be allowed to depend on modules/mediastream (now) and
modules/mediasource
> (later). Can you check if it's just a simple renaming?
> As stated on blink-dev, I think this could be put in
modules/srcobject, which
> would be allowed to depend on modules/mediastream (now) and
modules/mediasource
> (later). Can you check if it's just a simple renaming?
Moving this to modules/srcobject works fine.
git cl presubmit does not complain.
https://codereview.chromium.org/1815033003/
git cl presubmit does not complain.
https://codereview.chromium.org/1815033003/
phi...@opera.com
Mar 22, 2016, 12:06:55 PM3/22/16
to gui...@chromium.org, har...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.h
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.h#newcode111
third_party/WebKit/Source/core/html/HTMLMediaElement.h:111: void
setSrcObjectURL(const AtomicString&);
Just judging by include rules, maybe public/web/WebMediaProvider.h +
Source/web/WebMediaProvider.cpp would work?
As for HTMLMediaElement processing, the nicest thing would be an entry
point that matches
https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-load-resource
so that one can be confident that the implementation matches the spec.
One idea would be to let the WebMediaProvider represent both URLs and
all kinds of srcObjects and to use that throughout. Do you think that
could work?
Source/web/WebMediaProvider.cpp would work?
As for HTMLMediaElement processing, the nicest thing would be an entry
point that matches
https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-load-resource
so that one can be confident that the implementation matches the spec.
One idea would be to let the WebMediaProvider represent both URLs and
all kinds of srcObjects and to use that throughout. Do you think that
could work?
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
(right):
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp#newcode29
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp:29:
AtomicString
url(URLMediaStream::createObjectURL(element.getExecutionContext(),
mediaStream));
On 2016/03/21 16:34:53, Guido Urdaneta wrote:
> On 2016/03/21 11:13:13, philipj_UTC7 wrote:
> > If this URL is never revoked, won't the mediaStream object simply
leak?
> Getting
> > rid of this problem is one of the main benefits of srcObject over
> > createObjectURL, if it can be fixed.
>
> What if we do revoke the URL? For example, when srcObject becomes null
and
> perhaps in other cases?
> On 2016/03/21 11:13:13, philipj_UTC7 wrote:
> > If this URL is never revoked, won't the mediaStream object simply
leak?
> Getting
> > rid of this problem is one of the main benefits of srcObject over
> > createObjectURL, if it can be fixed.
>
> What if we do revoke the URL? For example, when srcObject becomes null
and
> perhaps in other cases?
Yes, if avoiding URLs is not possible right now, that'd be an
improvement.
https://codereview.chromium.org/1815033003/
improvement.
https://codereview.chromium.org/1815033003/
gui...@chromium.org
Mar 24, 2016, 10:38:45 AM3/24/16
to har...@chromium.org, philipj+...@opera.com, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
Hi, PTAL
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.h
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):
https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Source/core/html/HTMLMediaElement.h#newcode111
third_party/WebKit/Source/core/html/HTMLMediaElement.h:111: void
setSrcObjectURL(const AtomicString&);
I went for the URL+srcObject approach. I called the class
public/platform/WebMediaElementSource. I did not use WebMediaProvider
since the spec uses that term for the non-URL case. I'm open to naming
suggestions though :)
I couldn't exactly match
https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-load-resource
because that logic seems to be split across HTMLMediaElement and the
various WebMediaPlayers. Intead, I left the current loading logic
practically unmodified. Just ignoring URL checks when it's an srcObject.
Please let me know what you think.
https://codereview.chromium.org/1815033003/
public/platform/WebMediaElementSource. I did not use WebMediaProvider
since the spec uses that term for the non-URL case. I'm open to naming
suggestions though :)
I couldn't exactly match
https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-load-resource
because that logic seems to be split across HTMLMediaElement and the
various WebMediaPlayers. Intead, I left the current loading logic
practically unmodified. Just ignoring URL checks when it's an srcObject.
Please let me know what you think.
https://codereview.chromium.org/1815033003/
phi...@opera.com
Mar 24, 2016, 1:43:34 PM3/24/16
to gui...@chromium.org, har...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
phi...@opera.com
Mar 29, 2016, 9:28:51 AM3/29/16
to gui...@chromium.org, har...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
Can you add some reviewers for everything outside third_party/WebKit/ to get
that started?
https://codereview.chromium.org/1815033003/
that started?
https://codereview.chromium.org/1815033003/
phi...@opera.com
Mar 29, 2016, 9:43:20 AM3/29/16
to gui...@chromium.org, har...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
I haven't looked very closely at HTMLMediaElement yet.
https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/Source/core/html/HTMLMediaElement.h
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):
https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/Source/core/html/HTMLMediaElement.h#newcode360
third_party/WebKit/Source/core/html/HTMLMediaElement.h:360: void
loadResource(ContentType&);
Can loadResource still take the thing to load as an argument? If not,
why?
Note that this code is being refactored in
https://codereview.chromium.org/1810513002/ so there will be conflicts
for someone.
https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/public/platform/WebMediaElementSource.h
File third_party/WebKit/public/platform/WebMediaElementSource.h (right):
https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/public/platform/WebMediaElementSource.h#newcode33
third_party/WebKit/public/platform/WebMediaElementSource.h:33:
WebMediaStream m_mediaStream;
Is it possible to get the original MediaStream back from a
WebMediaStream? If it is, then it should be possible to let
WebMediaElementSource be the only thing that keeps it alive, which would
be more like how I hope this will finally look like, and will also mean
that HTMLMediaElementSrcObject doesn't need to be a supplement since the
HTMLMediaElement itself will hold on to the WebMediaElementSource
object.
https://codereview.chromium.org/1815033003/
https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/Source/core/html/HTMLMediaElement.h
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):
https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/Source/core/html/HTMLMediaElement.h#newcode360
third_party/WebKit/Source/core/html/HTMLMediaElement.h:360: void
loadResource(ContentType&);
Can loadResource still take the thing to load as an argument? If not,
why?
Note that this code is being refactored in
https://codereview.chromium.org/1810513002/ so there will be conflicts
for someone.
https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/public/platform/WebMediaElementSource.h
File third_party/WebKit/public/platform/WebMediaElementSource.h (right):
https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/public/platform/WebMediaElementSource.h#newcode33
third_party/WebKit/public/platform/WebMediaElementSource.h:33:
WebMediaStream m_mediaStream;
Is it possible to get the original MediaStream back from a
WebMediaStream? If it is, then it should be possible to let
WebMediaElementSource be the only thing that keeps it alive, which would
be more like how I hope this will finally look like, and will also mean
that HTMLMediaElementSrcObject doesn't need to be a supplement since the
HTMLMediaElement itself will hold on to the WebMediaElementSource
object.
https://codereview.chromium.org/1815033003/
gui...@chromium.org
Mar 29, 2016, 9:45:18 AM3/29/16
to har...@chromium.org, philipj+...@opera.com, to...@chromium.org, joc...@chromium.org, wole...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
to...@chromium.org: Please review changes in content/renderer/media
joc...@chromium.org: Please review changes in content/public, content/shell and
content/renderer (feel free to review anything else, of course)
wole...@chromium.org: Please review changes in media/blink
https://codereview.chromium.org/1815033003/
joc...@chromium.org: Please review changes in content/public, content/shell and
content/renderer (feel free to review anything else, of course)
wole...@chromium.org: Please review changes in media/blink
https://codereview.chromium.org/1815033003/
gui...@chromium.org
Mar 29, 2016, 10:48:11 AM3/29/16
to har...@chromium.org, philipj+...@opera.com, to...@chromium.org, joc...@chromium.org, wole...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/Source/core/html/HTMLMediaElement.h
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):
https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/Source/core/html/HTMLMediaElement.h#newcode360
third_party/WebKit/Source/core/html/HTMLMediaElement.h:360: void
loadResource(ContentType&);
On 2016/03/29 13:43:19, philipj_UTC7 wrote:
> Can loadResource still take the thing to load as an argument? If not,
why?
> Can loadResource still take the thing to load as an argument? If not,
why?
The thing to load is now stored in the m_source member.
When it's an srcObject it has to be stored somewhere because there are
asynchronous tasks between setSrcObject() and loadResource().
When it's an srcObject it has to be stored somewhere because there are
asynchronous tasks between setSrcObject() and loadResource().
> Note that this code is being refactored in
> https://codereview.chromium.org/1810513002/ so there will be conflicts
for
> someone.
I think rebasing on top of that CL will not be an issue.
https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/public/platform/WebMediaElementSource.h
File third_party/WebKit/public/platform/WebMediaElementSource.h (right):
https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/public/platform/WebMediaElementSource.h#newcode33
third_party/WebKit/public/platform/WebMediaElementSource.h:33:
WebMediaStream m_mediaStream;
On 2016/03/29 13:43:19, philipj_UTC7 wrote:
> Is it possible to get the original MediaStream back from a
WebMediaStream? If it
> is, then it should be possible to let WebMediaElementSource be the
only thing
> that keeps it alive, which would be more like how I hope this will
finally look
> like, and will also mean that HTMLMediaElementSrcObject doesn't need
to be a
> supplement since the HTMLMediaElement itself will hold on to the
> WebMediaElementSource object.
> Is it possible to get the original MediaStream back from a
WebMediaStream? If it
> is, then it should be possible to let WebMediaElementSource be the
only thing
> that keeps it alive, which would be more like how I hope this will
finally look
> like, and will also mean that HTMLMediaElementSrcObject doesn't need
to be a
> supplement since the HTMLMediaElement itself will hold on to the
> WebMediaElementSource object.
I just noticed it is possible to get the MediaStream from WebMediaStream
as a MediaStreamDescriptorClient. Updated HTMLMediaElementSrcObject to
no longer be a supplement.
https://codereview.chromium.org/1815033003/
as a MediaStreamDescriptorClient. Updated HTMLMediaElementSrcObject to
no longer be a supplement.
https://codereview.chromium.org/1815033003/
wole...@chromium.org
Mar 31, 2016, 3:35:27 PM3/31/16
to gui...@chromium.org, har...@chromium.org, philipj+...@opera.com, to...@chromium.org, joc...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
blink/media mostly looking good % nit.
Note, you'll also need to update
content/renderer/media/android/webmediaplayer_android.{h,cc} ("WMPA")
My l*tm is pending WMPA change and a question in HTMLMediaElement.cpp:
https://codereview.chromium.org/1815033003/diff/280001/media/blink/webmediaplayer_impl.cc
File media/blink/webmediaplayer_impl.cc (right):
https://codereview.chromium.org/1815033003/diff/280001/media/blink/webmediaplayer_impl.cc#newcode271
media/blink/webmediaplayer_impl.cc:271: // Only URL is supported.
nit: please s/is supported/or MSE blob URL are supported/.
https://codereview.chromium.org/1815033003/diff/280001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
https://codereview.chromium.org/1815033003/diff/280001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode563
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:563: if
(!getAttribute(srcAttr).isEmpty() && m_networkState == NETWORK_EMPTY) {
On node insertion, if srcObject is used instead of srcAttr, could
srcAttr still be empty here? If so, I suspect a further condition is
necessary here on srcObject to allow load to occur of MediaStream
srcObject to occur on node insertion.
https://codereview.chromium.org/1815033003/
Note, you'll also need to update
content/renderer/media/android/webmediaplayer_android.{h,cc} ("WMPA")
My l*tm is pending WMPA change and a question in HTMLMediaElement.cpp:
https://codereview.chromium.org/1815033003/diff/280001/media/blink/webmediaplayer_impl.cc
File media/blink/webmediaplayer_impl.cc (right):
https://codereview.chromium.org/1815033003/diff/280001/media/blink/webmediaplayer_impl.cc#newcode271
media/blink/webmediaplayer_impl.cc:271: // Only URL is supported.
nit: please s/is supported/or MSE blob URL are supported/.
https://codereview.chromium.org/1815033003/diff/280001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
https://codereview.chromium.org/1815033003/diff/280001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode563
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:563: if
(!getAttribute(srcAttr).isEmpty() && m_networkState == NETWORK_EMPTY) {
On node insertion, if srcObject is used instead of srcAttr, could
srcAttr still be empty here? If so, I suspect a further condition is
necessary here on srcObject to allow load to occur of MediaStream
srcObject to occur on node insertion.
https://codereview.chromium.org/1815033003/
gui...@chromium.org
Apr 1, 2016, 8:14:58 AM4/1/16
to har...@chromium.org, philipj+...@opera.com, to...@chromium.org, joc...@chromium.org, wole...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
wolenetz: note that changes to WMPA were already in the CL. Can you also review
the rest of content/renderer/media?
the rest of content/renderer/media?
https://codereview.chromium.org/1815033003/diff/280001/media/blink/webmediaplayer_impl.cc
File media/blink/webmediaplayer_impl.cc (right):
https://codereview.chromium.org/1815033003/diff/280001/media/blink/webmediaplayer_impl.cc#newcode271
media/blink/webmediaplayer_impl.cc:271: // Only URL is supported.
On 2016/03/31 19:35:27, wolenetz wrote:
> nit: please s/is supported/or MSE blob URL are supported/.
> nit: please s/is supported/or MSE blob URL are supported/.
Done.
https://codereview.chromium.org/1815033003/diff/280001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
https://codereview.chromium.org/1815033003/diff/280001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode563
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:563: if
(!getAttribute(srcAttr).isEmpty() && m_networkState == NETWORK_EMPTY) {
On 2016/03/31 19:35:27, wolenetz wrote:
> On node insertion, if srcObject is used instead of srcAttr, could
srcAttr still
> be empty here? If so, I suspect a further condition is necessary here
on
> srcObject to allow load to occur of MediaStream srcObject to occur on
node
> insertion.
> On node insertion, if srcObject is used instead of srcAttr, could
srcAttr still
> be empty here? If so, I suspect a further condition is necessary here
on
> srcObject to allow load to occur of MediaStream srcObject to occur on
node
> insertion.
to...@chromium.org
Apr 1, 2016, 9:20:40 AM4/1/16
to gui...@chromium.org, har...@chromium.org, philipj+...@opera.com, joc...@chromium.org, wole...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
lgtm for content/renderer/media
https://codereview.chromium.org/1815033003/diff/300001/content/renderer/media/web_media_element_source_utils.cc
File content/renderer/media/web_media_element_source_utils.cc (right):
https://codereview.chromium.org/1815033003/diff/300001/content/renderer/media/web_media_element_source_utils.cc#newcode18
content/renderer/media/web_media_element_source_utils.cc:18: if
(source.isURL())
nit: {}
https://codereview.chromium.org/1815033003/
https://codereview.chromium.org/1815033003/diff/300001/content/renderer/media/web_media_element_source_utils.cc
File content/renderer/media/web_media_element_source_utils.cc (right):
https://codereview.chromium.org/1815033003/diff/300001/content/renderer/media/web_media_element_source_utils.cc#newcode18
content/renderer/media/web_media_element_source_utils.cc:18: if
(source.isURL())
nit: {}
https://codereview.chromium.org/1815033003/
gui...@chromium.org
Apr 1, 2016, 10:17:34 AM4/1/16
to har...@chromium.org, philipj+...@opera.com, joc...@chromium.org, wole...@chromium.org, to...@chromium.org, xhw...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
+xhwang
xhwang: can you take a look at content/rendere/media/webmediaplayer_ms.cc lines
134 and 135?
You recently introduced a metric report that includes the MediaStream URL, but
now there might not be a URL for the MediaStream. I opted to report an empty URL
when this happens, but would like your input on this.
https://codereview.chromium.org/1815033003/
xhwang: can you take a look at content/rendere/media/webmediaplayer_ms.cc lines
134 and 135?
You recently introduced a metric report that includes the MediaStream URL, but
now there might not be a URL for the MediaStream. I opted to report an empty URL
when this happens, but would like your input on this.
https://codereview.chromium.org/1815033003/
h...@chromium.org
Apr 1, 2016, 11:08:16 AM4/1/16
to gui...@chromium.org, har...@chromium.org, philipj+...@opera.com, joc...@chromium.org, wole...@chromium.org, to...@chromium.org, xhw...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org
Added a few comments on layout tests.
https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
(right):
https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode24
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:24:
function playingFileOnce()
Style suggestion: name function playingFileFromSrcAttribute (the
critical thing isn't that it's played only once)
https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode43
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:43:
video.srcObject=null;
Nit: Spaces around operator
https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode46
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:46:
function playingFileTwice()
Style suggestion: Name function playing FileAfterSrcObjectRemoved
https://codereview.chromium.org/1815033003/
https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
(right):
https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode24
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:24:
function playingFileOnce()
Style suggestion: name function playingFileFromSrcAttribute (the
critical thing isn't that it's played only once)
https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode43
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:43:
video.srcObject=null;
Nit: Spaces around operator
https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode46
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:46:
function playingFileTwice()
Style suggestion: Name function playing FileAfterSrcObjectRemoved
https://codereview.chromium.org/1815033003/
gui...@chromium.org
Apr 2, 2016, 2:32:52 PM4/2/16
to har...@chromium.org, philipj+...@opera.com, joc...@chromium.org, wole...@chromium.org, to...@chromium.org, xhw...@chromium.org, h...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
(right):
https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode24
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:24:
function playingFileOnce()
On 2016/04/01 15:08:15, hta - Chromium wrote:
> Style suggestion: name function playingFileFromSrcAttribute (the
critical thing
> isn't that it's played only once)
> Style suggestion: name function playingFileFromSrcAttribute (the
critical thing
> isn't that it's played only once)
Done.
https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode43
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:43:
video.srcObject=null;
On 2016/04/01 15:08:15, hta - Chromium wrote:
> Nit: Spaces around operator
Done.
> Nit: Spaces around operator
Done.
https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode46
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:46:
function playingFileTwice()
On 2016/04/01 15:08:15, hta - Chromium wrote:
> Style suggestion: Name function playing FileAfterSrcObjectRemoved
> Style suggestion: Name function playing FileAfterSrcObjectRemoved
phi...@opera.com
Apr 6, 2016, 7:58:42 AM4/6/16
to gui...@chromium.org, har...@chromium.org, joc...@chromium.org, wole...@chromium.org, to...@chromium.org, xhw...@chromium.org, h...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
Looks like we're on the right track, please see comments. The new module needs
an OWNER, I guess that's you :)
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
(right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode5
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:5:
<div id="log"></div>
You typically don't need to include this, it'll be inserted
automatically.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode14
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:14:
var video = document.getElementById("testVideo");
You can skip the id and just use document.querySelector("video"). As
little markup and code as possible in tests is nice, gets right to the
important bits :)
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode15
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:15:
var trackevent = "playing";
Just inline "playing" everywhere?
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode21
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:21:
video.src = getVideoURI("test");
It'd be more clear if you set src and srcObject at the same time I
think. If you're trying to test something about the timing by doing at
separate points, being more explicit about that would be good, but I
don't think that's the case.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
(right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode4
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:4:
<video id="testVideo" autoplay="autoplay"></video>
Nit: The autoplay attribute doesn't need any specific value, the empty
string is fine, which you'll get by just <video autoplay>.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode6
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:6:
<p>Test assigment of a mediastream via the srcObject attribute.</p>
Can you put this inside <title> at the top? This way, when there's only
a single test, that test gets the document.title as its title by
default. Also capitalize MediaStream.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode9
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:9:
<script src="./w3c-media-utils.js"></script>
Doesn't look like you're using anything from w3c-media-utils.js.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode16
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:16:
test.step(() => assert_idl_attribute(video, "srcObject"));
You shouldn't need to wrap any individual steps like this. Just make
sure that each of these functions is appropriately wrapped in
test.step_func(). This way if exceptions are thrown where you're not
anticipating them, like say if navigator.webkitGetUserMedia is removed,
it'll still fail the test properly.
You can avoid the startTest() method entirely by just doing
async_test(test => { /* the steps that start at this line */ }, /* no
title if you have a <title> above */).
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode17
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:17:
video.addEventListener("playing", playingSrcObject);
Here you'll need test.step_func(playingSrcObject). Similarly elsewhere.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode22
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:22:
stream => video.srcObject = stream,
The sequence of events could be a bit clearer if you added the playing
event listener in this callback, because I guess it's setting the
srcObject which will eventually cause the play to happen? An explicit
call to play() and removing autoplay would also be clearer, IMHO.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode23
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:23:
_ => test.step(() => assert_unreached("Did not get mediastream")));
This can be test.unreached_func("Did not get MediaStream");
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode28
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:28:
video.removeEventListener("playing", playingSrcObject)
Is this needed? If it is, then using the onplaying attribute and setting
it to null here could be cleaner than saving the
test.step_func(playingSrcObject) value that you'd otherwise have to do.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode29
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:29:
video.addEventListener("emptied", playingNothing)
This is an oddly named callback. If you don't enjoy coming up with names
for callbacks, you can of course just nest everything. Go down to two
spaces indent and it may still be readable enough.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode33
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:33:
video.srcObject=null;
Needs more whitespace.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode40
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:40:
test.done();
When this is wrapped instead, you can use test.step_func_done() and omit
the step.done().
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode627
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:627: if
(m_networkState == NETWORK_EMPTY && (!getAttribute(srcAttr).isEmpty() ||
m_source.isMediaProviderObject())) {
Hmm, why this change? This block of code isn't per spec, did something
break if you didn't extend it to srcObject?
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode727
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:727: m_source =
srcObject;
Looking at this I'm thinking that it'd be cleaner if this is called just
m_srcObject and that before setting it you
ASSERT(srcObject.isMediaProviderObject()).
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode999
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:999: m_source =
WebMediaElementSource(WebURL(mediaURL));
Here, rather than setting m_source/m_srcObject, can you add back
loadResource's first argument and just pass it in here? I guess it can
be const WebMediaPlayerSource& and the WebMediaPlayer doesn't get any
ownership, so it has to extract the wrapped source from it before
returning.
This should force the code into being more like the spec, it won't be
possible to keep some extra state in m_source/m_srcObject, which could
be tempting even if you haven't done it here.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode1016
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1016: m_source
= WebMediaElementSource(WebURL(mediaURL));
Ditto.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode1065
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1065: bool
isBlobProtocol = m_source.isMediaProviderObject() ||
url.protocolIs(mediaSourceBlobProtocol);
I guess isObjectOrBlobURL or something would be more accurate?
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode1110
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1110: if
(m_source.isURL()) {
This will have to change with my proposed changes to m_srcObject. Will
you be able to use m_mode?
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp
File
third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp
(right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp#newcode21
third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp:21:
MediaStream* stream = static_cast<MediaStream*>(descriptor->client());
Can you introduce a free function toMediaStream(MediaStreamDescriptor*)
or a MediaStreamDescriptor::getAsMediaStream() instead so that the
static_cast is together with the code that makes it obviously safe?
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/public/platform/WebMediaElementSource.h
File third_party/WebKit/public/platform/WebMediaElementSource.h (right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/public/platform/WebMediaElementSource.h#newcode14
third_party/WebKit/public/platform/WebMediaElementSource.h:14: class
BLINK_PLATFORM_EXPORT WebMediaElementSource {
How about WebMediaPlayerSource as the name? It goes together with
WebMediaPlayer, and it's nice to contain the notion of a media element
(HTMLMediaElement) to inside Blink as much as possible.
https://codereview.chromium.org/1815033003/
an OWNER, I guess that's you :)
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
(right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode5
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:5:
<div id="log"></div>
You typically don't need to include this, it'll be inserted
automatically.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode14
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:14:
var video = document.getElementById("testVideo");
You can skip the id and just use document.querySelector("video"). As
little markup and code as possible in tests is nice, gets right to the
important bits :)
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode15
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:15:
var trackevent = "playing";
Just inline "playing" everywhere?
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode21
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:21:
video.src = getVideoURI("test");
It'd be more clear if you set src and srcObject at the same time I
think. If you're trying to test something about the timing by doing at
separate points, being more explicit about that would be good, but I
don't think that's the case.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
(right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode4
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:4:
<video id="testVideo" autoplay="autoplay"></video>
Nit: The autoplay attribute doesn't need any specific value, the empty
string is fine, which you'll get by just <video autoplay>.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode6
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:6:
<p>Test assigment of a mediastream via the srcObject attribute.</p>
Can you put this inside <title> at the top? This way, when there's only
a single test, that test gets the document.title as its title by
default. Also capitalize MediaStream.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode9
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:9:
<script src="./w3c-media-utils.js"></script>
Doesn't look like you're using anything from w3c-media-utils.js.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode16
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:16:
test.step(() => assert_idl_attribute(video, "srcObject"));
You shouldn't need to wrap any individual steps like this. Just make
sure that each of these functions is appropriately wrapped in
test.step_func(). This way if exceptions are thrown where you're not
anticipating them, like say if navigator.webkitGetUserMedia is removed,
it'll still fail the test properly.
You can avoid the startTest() method entirely by just doing
async_test(test => { /* the steps that start at this line */ }, /* no
title if you have a <title> above */).
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode17
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:17:
video.addEventListener("playing", playingSrcObject);
Here you'll need test.step_func(playingSrcObject). Similarly elsewhere.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode22
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:22:
stream => video.srcObject = stream,
The sequence of events could be a bit clearer if you added the playing
event listener in this callback, because I guess it's setting the
srcObject which will eventually cause the play to happen? An explicit
call to play() and removing autoplay would also be clearer, IMHO.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode23
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:23:
_ => test.step(() => assert_unreached("Did not get mediastream")));
This can be test.unreached_func("Did not get MediaStream");
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode28
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:28:
video.removeEventListener("playing", playingSrcObject)
Is this needed? If it is, then using the onplaying attribute and setting
it to null here could be cleaner than saving the
test.step_func(playingSrcObject) value that you'd otherwise have to do.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode29
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:29:
video.addEventListener("emptied", playingNothing)
This is an oddly named callback. If you don't enjoy coming up with names
for callbacks, you can of course just nest everything. Go down to two
spaces indent and it may still be readable enough.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode33
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:33:
video.srcObject=null;
Needs more whitespace.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode40
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:40:
test.done();
When this is wrapped instead, you can use test.step_func_done() and omit
the step.done().
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode627
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:627: if
(m_networkState == NETWORK_EMPTY && (!getAttribute(srcAttr).isEmpty() ||
m_source.isMediaProviderObject())) {
Hmm, why this change? This block of code isn't per spec, did something
break if you didn't extend it to srcObject?
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode727
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:727: m_source =
srcObject;
Looking at this I'm thinking that it'd be cleaner if this is called just
m_srcObject and that before setting it you
ASSERT(srcObject.isMediaProviderObject()).
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode999
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:999: m_source =
WebMediaElementSource(WebURL(mediaURL));
Here, rather than setting m_source/m_srcObject, can you add back
loadResource's first argument and just pass it in here? I guess it can
be const WebMediaPlayerSource& and the WebMediaPlayer doesn't get any
ownership, so it has to extract the wrapped source from it before
returning.
This should force the code into being more like the spec, it won't be
possible to keep some extra state in m_source/m_srcObject, which could
be tempting even if you haven't done it here.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode1016
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1016: m_source
= WebMediaElementSource(WebURL(mediaURL));
Ditto.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode1065
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1065: bool
isBlobProtocol = m_source.isMediaProviderObject() ||
url.protocolIs(mediaSourceBlobProtocol);
I guess isObjectOrBlobURL or something would be more accurate?
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode1110
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1110: if
(m_source.isURL()) {
This will have to change with my proposed changes to m_srcObject. Will
you be able to use m_mode?
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp
File
third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp
(right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp#newcode21
third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp:21:
MediaStream* stream = static_cast<MediaStream*>(descriptor->client());
Can you introduce a free function toMediaStream(MediaStreamDescriptor*)
or a MediaStreamDescriptor::getAsMediaStream() instead so that the
static_cast is together with the code that makes it obviously safe?
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/public/platform/WebMediaElementSource.h
File third_party/WebKit/public/platform/WebMediaElementSource.h (right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/public/platform/WebMediaElementSource.h#newcode14
third_party/WebKit/public/platform/WebMediaElementSource.h:14: class
BLINK_PLATFORM_EXPORT WebMediaElementSource {
How about WebMediaPlayerSource as the name? It goes together with
WebMediaPlayer, and it's nice to contain the notion of a media element
(HTMLMediaElement) to inside Blink as much as possible.
https://codereview.chromium.org/1815033003/
gui...@chromium.org
Apr 6, 2016, 12:34:54 PM4/6/16
to har...@chromium.org, philipj+...@opera.com, joc...@chromium.org, wole...@chromium.org, to...@chromium.org, xhw...@chromium.org, h...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
(right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode5
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:5:
<div id="log"></div>
On 2016/04/06 11:58:41, philipj wrote:
> You typically don't need to include this, it'll be inserted
automatically.
> You typically don't need to include this, it'll be inserted
automatically.
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode14
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:14:
var video = document.getElementById("testVideo");
On 2016/04/06 11:58:41, philipj wrote:
> You can skip the id and just use document.querySelector("video"). As
little
> markup and code as possible in tests is nice, gets right to the
important bits
> :)
> You can skip the id and just use document.querySelector("video"). As
little
> markup and code as possible in tests is nice, gets right to the
important bits
> :)
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode15
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:15:
var trackevent = "playing";
On 2016/04/06 11:58:41, philipj wrote:
> Just inline "playing" everywhere?
Done. Using onplaying instead.
> Just inline "playing" everywhere?
Done. Using onplaying instead.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode21
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:21:
video.src = getVideoURI("test");
On 2016/04/06 11:58:41, philipj wrote:
> It'd be more clear if you set src and srcObject at the same time I
think. If
> you're trying to test something about the timing by doing at separate
points,
> being more explicit about that would be good, but I don't think that's
the case.
> It'd be more clear if you set src and srcObject at the same time I
think. If
> you're trying to test something about the timing by doing at separate
points,
> being more explicit about that would be good, but I don't think that's
the case.
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
(right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode4
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:4:
<video id="testVideo" autoplay="autoplay"></video>
On 2016/04/06 11:58:42, philipj wrote:
> Nit: The autoplay attribute doesn't need any specific value, the empty
string is
> fine, which you'll get by just <video autoplay>.
> Nit: The autoplay attribute doesn't need any specific value, the empty
string is
> fine, which you'll get by just <video autoplay>.
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode6
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:6:
<p>Test assigment of a mediastream via the srcObject attribute.</p>
On 2016/04/06 11:58:42, philipj wrote:
> Can you put this inside <title> at the top? This way, when there's
only a single
> test, that test gets the document.title as its title by default. Also
capitalize
> MediaStream.
> Can you put this inside <title> at the top? This way, when there's
only a single
> test, that test gets the document.title as its title by default. Also
capitalize
> MediaStream.
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode9
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:9:
<script src="./w3c-media-utils.js"></script>
On 2016/04/06 11:58:41, philipj wrote:
> Doesn't look like you're using anything from w3c-media-utils.js.
> Doesn't look like you're using anything from w3c-media-utils.js.
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode16
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:16:
test.step(() => assert_idl_attribute(video, "srcObject"));
On 2016/04/06 11:58:42, philipj wrote:
> You shouldn't need to wrap any individual steps like this. Just make
sure that
> each of these functions is appropriately wrapped in test.step_func().
This way
> if exceptions are thrown where you're not anticipating them, like say
if
> navigator.webkitGetUserMedia is removed, it'll still fail the test
properly.
>
> You can avoid the startTest() method entirely by just doing
async_test(test => {
> /* the steps that start at this line */ }, /* no title if you have a
<title>
> above */).
> You shouldn't need to wrap any individual steps like this. Just make
sure that
> each of these functions is appropriately wrapped in test.step_func().
This way
> if exceptions are thrown where you're not anticipating them, like say
if
> navigator.webkitGetUserMedia is removed, it'll still fail the test
properly.
>
> You can avoid the startTest() method entirely by just doing
async_test(test => {
> /* the steps that start at this line */ }, /* no title if you have a
<title>
> above */).
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode17
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:17:
video.addEventListener("playing", playingSrcObject);
On 2016/04/06 11:58:41, philipj wrote:
> Here you'll need test.step_func(playingSrcObject). Similarly
elsewhere.
> Here you'll need test.step_func(playingSrcObject). Similarly
elsewhere.
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode22
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:22:
stream => video.srcObject = stream,
On 2016/04/06 11:58:42, philipj wrote:
> The sequence of events could be a bit clearer if you added the playing
event
> listener in this callback, because I guess it's setting the srcObject
which will
> eventually cause the play to happen? An explicit call to play() and
removing
> autoplay would also be clearer, IMHO.
> The sequence of events could be a bit clearer if you added the playing
event
> listener in this callback, because I guess it's setting the srcObject
which will
> eventually cause the play to happen? An explicit call to play() and
removing
> autoplay would also be clearer, IMHO.
Done, but kept the autoplay. With an explicit call to play() the test
worked, but I got the following console message:
"Uncaught (in promise) AbortError: The play() request was interrupted by
a new load request."
worked, but I got the following console message:
"Uncaught (in promise) AbortError: The play() request was interrupted by
a new load request."
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode23
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:23:
_ => test.step(() => assert_unreached("Did not get mediastream")));
On 2016/04/06 11:58:41, philipj wrote:
> This can be test.unreached_func("Did not get MediaStream");
> This can be test.unreached_func("Did not get MediaStream");
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode28
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:28:
video.removeEventListener("playing", playingSrcObject)
On 2016/04/06 11:58:42, philipj wrote:
> Is this needed? If it is, then using the onplaying attribute and
setting it to
> null here could be cleaner than saving the
test.step_func(playingSrcObject)
> value that you'd otherwise have to do.
> Is this needed? If it is, then using the onplaying attribute and
setting it to
> null here could be cleaner than saving the
test.step_func(playingSrcObject)
> value that you'd otherwise have to do.
Done. Not needed.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode29
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:29:
video.addEventListener("emptied", playingNothing)
On 2016/04/06 11:58:41, philipj wrote:
> This is an oddly named callback. If you don't enjoy coming up with
names for
> callbacks, you can of course just nest everything. Go down to two
spaces indent
> and it may still be readable enough.
> This is an oddly named callback. If you don't enjoy coming up with
names for
> callbacks, you can of course just nest everything. Go down to two
spaces indent
> and it may still be readable enough.
Done. What would have been a good name? :)
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode33
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:33:
video.srcObject=null;
On 2016/04/06 11:58:42, philipj wrote:
> Needs more whitespace.
Done.
> Needs more whitespace.
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode40
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:40:
test.done();
On 2016/04/06 11:58:42, philipj wrote:
> When this is wrapped instead, you can use test.step_func_done() and
omit the
> step.done().
> When this is wrapped instead, you can use test.step_func_done() and
omit the
> step.done().
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode627
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:627: if
(m_networkState == NETWORK_EMPTY && (!getAttribute(srcAttr).isEmpty() ||
m_source.isMediaProviderObject())) {
On 2016/04/06 11:58:42, philipj wrote:
> Hmm, why this change? This block of code isn't per spec, did something
break if
> you didn't extend it to srcObject?
> Hmm, why this change? This block of code isn't per spec, did something
break if
> you didn't extend it to srcObject?
Nothing broke with or without this change.
wolenetz@ suggested this might be necessary and it made sense to me.
I reverted it for now.
wolenetz@ suggested this might be necessary and it made sense to me.
I reverted it for now.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode727
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:727: m_source =
srcObject;
On 2016/04/06 11:58:42, philipj wrote:
> Looking at this I'm thinking that it'd be cleaner if this is called
just
> m_srcObject and that before setting it you
> ASSERT(srcObject.isMediaProviderObject()).
> Looking at this I'm thinking that it'd be cleaner if this is called
just
> m_srcObject and that before setting it you
> ASSERT(srcObject.isMediaProviderObject()).
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode999
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:999: m_source =
WebMediaElementSource(WebURL(mediaURL));
On 2016/04/06 11:58:42, philipj wrote:
> Here, rather than setting m_source/m_srcObject, can you add back
loadResource's
> first argument and just pass it in here? I guess it can be const
> WebMediaPlayerSource& and the WebMediaPlayer doesn't get any
ownership, so it
> has to extract the wrapped source from it before returning.
>
> This should force the code into being more like the spec, it won't be
possible
> to keep some extra state in m_source/m_srcObject, which could be
tempting even
> if you haven't done it here.
> Here, rather than setting m_source/m_srcObject, can you add back
loadResource's
> first argument and just pass it in here? I guess it can be const
> WebMediaPlayerSource& and the WebMediaPlayer doesn't get any
ownership, so it
> has to extract the wrapped source from it before returning.
>
> This should force the code into being more like the spec, it won't be
possible
> to keep some extra state in m_source/m_srcObject, which could be
tempting even
> if you haven't done it here.
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode1016
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1016: m_source
= WebMediaElementSource(WebURL(mediaURL));
On 2016/04/06 11:58:42, philipj wrote:
> Ditto.
Done.
> Ditto.
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode1065
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1065: bool
isBlobProtocol = m_source.isMediaProviderObject() ||
url.protocolIs(mediaSourceBlobProtocol);
On 2016/04/06 11:58:42, philipj wrote:
> I guess isObjectOrBlobURL or something would be more accurate?
> I guess isObjectOrBlobURL or something would be more accurate?
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode1110
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1110: if
(m_source.isURL()) {
On 2016/04/06 11:58:42, philipj wrote:
> This will have to change with my proposed changes to m_srcObject. Will
you be
> able to use m_mode?
> This will have to change with my proposed changes to m_srcObject. Will
you be
> able to use m_mode?
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp
File
third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp
(right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp#newcode21
third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp:21:
MediaStream* stream = static_cast<MediaStream*>(descriptor->client());
On 2016/04/06 11:58:42, philipj wrote:
> Can you introduce a free function
toMediaStream(MediaStreamDescriptor*) or a
> MediaStreamDescriptor::getAsMediaStream() instead so that the
static_cast is
> together with the code that makes it obviously safe?
> Can you introduce a free function
toMediaStream(MediaStreamDescriptor*) or a
> MediaStreamDescriptor::getAsMediaStream() instead so that the
static_cast is
> together with the code that makes it obviously safe?
Done.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/public/platform/WebMediaElementSource.h
File third_party/WebKit/public/platform/WebMediaElementSource.h (right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/public/platform/WebMediaElementSource.h#newcode14
third_party/WebKit/public/platform/WebMediaElementSource.h:14: class
BLINK_PLATFORM_EXPORT WebMediaElementSource {
On 2016/04/06 11:58:42, philipj wrote:
> How about WebMediaPlayerSource as the name? It goes together with
> WebMediaPlayer, and it's nice to contain the notion of a media element
> (HTMLMediaElement) to inside Blink as much as possible.
> How about WebMediaPlayerSource as the name? It goes together with
> WebMediaPlayer, and it's nice to contain the notion of a media element
> (HTMLMediaElement) to inside Blink as much as possible.
phi...@opera.com
Apr 7, 2016, 11:38:11 AM4/7/16
to gui...@chromium.org, har...@chromium.org, joc...@chromium.org, wole...@chromium.org, to...@chromium.org, xhw...@chromium.org, h...@chromium.org, chromium...@chromium.org, blink-rev...@chromium.org, tommyw+w...@chromium.org, mlamouri+w...@chromium.org, gl...@chromium.org, f...@opera.com, eric.c...@apple.com, feature-me...@chromium.org, dglazko...@chromium.org, silv...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, vcar...@chromium.org, h...@chromium.org
third_party/WebKit/ LGTM % a few questions and nits.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
(right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode22
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:22:
stream => video.srcObject = stream,
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:22:
stream => video.srcObject = stream,
On 2016/04/06 16:34:54, Guido Urdaneta wrote:
> On 2016/04/06 11:58:42, philipj wrote:
> > The sequence of events could be a bit clearer if you added the
playing event
> > listener in this callback, because I guess it's setting the
srcObject which
> will
> > eventually cause the play to happen? An explicit call to play() and
removing
> > autoplay would also be clearer, IMHO.
>
> Done, but kept the autoplay. With an explicit call to play() the test
worked,
> but I got the following console message:
> "Uncaught (in promise) AbortError: The play() request was interrupted
by a new
> load request."
> On 2016/04/06 11:58:42, philipj wrote:
> > The sequence of events could be a bit clearer if you added the
playing event
> > listener in this callback, because I guess it's setting the
srcObject which
> will
> > eventually cause the play to happen? An explicit call to play() and
removing
> > autoplay would also be clearer, IMHO.
>
> Done, but kept the autoplay. With an explicit call to play() the test
worked,
> but I got the following console message:
> "Uncaught (in promise) AbortError: The play() request was interrupted
by a new
> load request."
Do you get a rejected promise even if you call play() after setting
srcObject? I think that should be fixed by a CL mlamouri@ is working on:
https://codereview.chromium.org/1865933002/
Just using autoplay is fine, though.
srcObject? I think that should be fixed by a CL mlamouri@ is working on:
https://codereview.chromium.org/1865933002/
Just using autoplay is fine, though.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode29
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:29:
video.addEventListener("emptied", playingNothing)
On 2016/04/06 16:34:54, Guido Urdaneta wrote:
> On 2016/04/06 11:58:41, philipj wrote:
> > This is an oddly named callback. If you don't enjoy coming up with
names for
> > callbacks, you can of course just nest everything. Go down to two
spaces
> indent
> > and it may still be readable enough.
>
> Done. What would have been a good name? :)
> On 2016/04/06 11:58:41, philipj wrote:
> > This is an oddly named callback. If you don't enjoy coming up with
names for
> > callbacks, you can of course just nest everything. Go down to two
spaces
> indent
> > and it may still be readable enough.
>
> Done. What would have been a good name? :)
Maybe "emptied"? Just "playing" for the current function would also
work.
work.
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode627
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:627: if
(m_networkState == NETWORK_EMPTY && (!getAttribute(srcAttr).isEmpty() ||
m_source.isMediaProviderObject())) {
On 2016/04/06 16:34:54, Guido Urdaneta wrote:
> On 2016/04/06 11:58:42, philipj wrote:
> > Hmm, why this change? This block of code isn't per spec, did
something break
> if
> > you didn't extend it to srcObject?
>
> Nothing broke with or without this change.
> wolenetz@ suggested this might be necessary and it made sense to me.
> I reverted it for now.
> On 2016/04/06 11:58:42, philipj wrote:
> > Hmm, why this change? This block of code isn't per spec, did
something break
> if
> > you didn't extend it to srcObject?
>
> Nothing broke with or without this change.
> wolenetz@ suggested this might be necessary and it made sense to me.
> I reverted it for now.
What could now be broken when using srcObject is moving a media element
between documents. Even with the change it wouldn't work per spec, but
it would work a little bit. So if you want to write a test for it and
think it makes sense, I'm OK with that.
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
(right):
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode10
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:10:
var test = async_test(test => {
I'd suggest the same kinds of changes here, to contain everything inside
the async_test
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
(right):
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode9
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:9:
var test = async_test(test => {
This looks a bit unusual, if you just move playingSrcObject into the
inner function you won't need the return value, and you can move the
video variable in as well.
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp
File
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp
(right):
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp#newcode28
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp:28:
bool WebMediaPlayerSource::isEmpty() const
Hmm, this isn't actually used anywhere?
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp#newcode43
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp:43:
bool WebMediaPlayerSource::isMediaProviderObject() const
Maybe for the time being, just isMediaStream() will suffice? They do the
same thing now, and in HTMLMediaElement::loadResource there's actually a
bit of redundancy if one knows this.
https://codereview.chromium.org/1815033003/
between documents. Even with the change it wouldn't work per spec, but
it would work a little bit. So if you want to write a test for it and
think it makes sense, I'm OK with that.
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
(right):
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode10
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:10:
var test = async_test(test => {
I'd suggest the same kinds of changes here, to contain everything inside
the async_test
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
(right):
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode9
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:9:
var test = async_test(test => {
This looks a bit unusual, if you just move playingSrcObject into the
inner function you won't need the return value, and you can move the
video variable in as well.
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp
File
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp
(right):
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp#newcode28
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp:28:
bool WebMediaPlayerSource::isEmpty() const
Hmm, this isn't actually used anywhere?
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp#newcode43
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp:43:
bool WebMediaPlayerSource::isMediaProviderObject() const
Maybe for the time being, just isMediaStream() will suffice? They do the
same thing now, and in HTMLMediaElement::loadResource there's actually a
bit of redundancy if one knows this.
https://codereview.chromium.org/1815033003/
joc...@chromium.org
Apr 7, 2016, 10:22:35 PM4/7/16
to gui...@chromium.org, har...@chromium.org, h...@chromium.org, philipj+...@opera.com, to...@chromium.org, wole...@chromium.org, xhw...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, f...@opera.com, gl...@chromium.org, h...@chromium.org, kinuko...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, silv...@chromium.org, tommyw+w...@chromium.org, vcar...@chromium.org
gui...@chromium.org
Apr 8, 2016, 6:58:47 AM4/8/16
to joc...@chromium.org, har...@chromium.org, h...@chromium.org, philipj+...@opera.com, to...@chromium.org, wole...@chromium.org, xhw...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, f...@opera.com, gl...@chromium.org, h...@chromium.org, kinuko...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, silv...@chromium.org, tommyw+w...@chromium.org, vcar...@chromium.org
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode627
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:627: if
(m_networkState == NETWORK_EMPTY && (!getAttribute(srcAttr).isEmpty() ||
m_source.isMediaProviderObject())) {
On 2016/04/07 15:38:11, philipj wrote:
> On 2016/04/06 16:34:54, Guido Urdaneta wrote:
> > On 2016/04/06 11:58:42, philipj wrote:
> > > Hmm, why this change? This block of code isn't per spec, did
something break
> > if
> > > you didn't extend it to srcObject?
> >
> > Nothing broke with or without this change.
> > wolenetz@ suggested this might be necessary and it made sense to me.
> > I reverted it for now.
>
> What could now be broken when using srcObject is moving a media
element between
> documents. Even with the change it wouldn't work per spec, but it
would work a
> little bit. So if you want to write a test for it and think it makes
sense, I'm
> OK with that.
> On 2016/04/06 16:34:54, Guido Urdaneta wrote:
> > On 2016/04/06 11:58:42, philipj wrote:
> > > Hmm, why this change? This block of code isn't per spec, did
something break
> > if
> > > you didn't extend it to srcObject?
> >
> > Nothing broke with or without this change.
> > wolenetz@ suggested this might be necessary and it made sense to me.
> > I reverted it for now.
>
> What could now be broken when using srcObject is moving a media
element between
> documents. Even with the change it wouldn't work per spec, but it
would work a
> little bit. So if you want to write a test for it and think it makes
sense, I'm
> OK with that.
I think it makes sense to add the srcObject check to the condition.
However, I was unable to hit this condition. Whenever I tried moving an
element with nonempty src or srcObject, the networkState was never
NETWORK_EMPTY.
Nevertheless, I added a layout test for moving an element with an
srcObject to a new document. It works regardless of this change.
However, I was unable to hit this condition. Whenever I tried moving an
element with nonempty src or srcObject, the networkState was never
NETWORK_EMPTY.
Nevertheless, I added a layout test for moving an element with an
srcObject to a new document. It works regardless of this change.
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
(right):
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html#newcode10
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:10:
var test = async_test(test => {
On 2016/04/07 15:38:11, philipj wrote:
> I'd suggest the same kinds of changes here, to contain everything
inside the
> async_test
> I'd suggest the same kinds of changes here, to contain everything
inside the
> async_test
Done.
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
(right):
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html#newcode9
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:9:
var test = async_test(test => {
On 2016/04/07 15:38:11, philipj wrote:
> This looks a bit unusual, if you just move playingSrcObject into the
inner
> function you won't need the return value, and you can move the video
variable in
> as well.
> This looks a bit unusual, if you just move playingSrcObject into the
inner
> function you won't need the return value, and you can move the video
variable in
> as well.
Done.
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp
File
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp
(right):
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp#newcode28
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp:28:
bool WebMediaPlayerSource::isEmpty() const
On 2016/04/07 15:38:11, philipj wrote:
> Hmm, this isn't actually used anywhere?
> Hmm, this isn't actually used anywhere?
Removed. Will add it in the future if we need it.
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp#newcode43
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp:43:
bool WebMediaPlayerSource::isMediaProviderObject() const
On 2016/04/07 15:38:11, philipj wrote:
> Maybe for the time being, just isMediaStream() will suffice? They do
the same
> thing now, and in HTMLMediaElement::loadResource there's actually a
bit of
> redundancy if one knows this.
> Maybe for the time being, just isMediaStream() will suffice? They do
the same
> thing now, and in HTMLMediaElement::loadResource there's actually a
bit of
> redundancy if one knows this.
I think isMediaProviderObject() makes the code easier to follow in terms
of the spec. I don't see much benefit in removing it, since our plan is
to support the other types anyway.
https://codereview.chromium.org/1815033003/
of the spec. I don't see much benefit in removing it, since our plan is
to support the other types anyway.
https://codereview.chromium.org/1815033003/
gui...@chromium.org
Apr 8, 2016, 7:00:15 AM4/8/16
to joc...@chromium.org, har...@chromium.org, h...@chromium.org, philipj+...@opera.com, to...@chromium.org, wole...@chromium.org, xhw...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, f...@opera.com, gl...@chromium.org, h...@chromium.org, kinuko...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, silv...@chromium.org, tommyw+w...@chromium.org, vcar...@chromium.org
phi...@opera.com
Apr 8, 2016, 8:49:31 AM4/8/16
to gui...@chromium.org, joc...@chromium.org, har...@chromium.org, h...@chromium.org, to...@chromium.org, wole...@chromium.org, xhw...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, f...@opera.com, gl...@chromium.org, h...@chromium.org, kinuko...@chromium.org, mlamouri+w...@chromium.org, silv...@chromium.org, tommyw+w...@chromium.org, vcar...@chromium.org
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp
File
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp
(right):
https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp#newcode43
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp:43:
bool WebMediaPlayerSource::isMediaProviderObject() const
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp:43:
bool WebMediaPlayerSource::isMediaProviderObject() const
On 2016/04/08 10:58:47, Guido Urdaneta wrote:
> On 2016/04/07 15:38:11, philipj wrote:
> > Maybe for the time being, just isMediaStream() will suffice? They do
the same
> > thing now, and in HTMLMediaElement::loadResource there's actually a
bit of
> > redundancy if one knows this.
>
> I think isMediaProviderObject() makes the code easier to follow in
terms of the
> spec. I don't see much benefit in removing it, since our plan is to
support the
> other types anyway.
> On 2016/04/07 15:38:11, philipj wrote:
> > Maybe for the time being, just isMediaStream() will suffice? They do
the same
> > thing now, and in HTMLMediaElement::loadResource there's actually a
bit of
> > redundancy if one knows this.
>
> I think isMediaProviderObject() makes the code easier to follow in
terms of the
> spec. I don't see much benefit in removing it, since our plan is to
support the
> other types anyway.
wole...@chromium.org
Apr 8, 2016, 7:10:07 PM4/8/16
to gui...@chromium.org, joc...@chromium.org, har...@chromium.org, h...@chromium.org, philipj+...@opera.com, to...@chromium.org, xhw...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, f...@opera.com, gl...@chromium.org, h...@chromium.org, kinuko...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, silv...@chromium.org, tommyw+w...@chromium.org, vcar...@chromium.org
commit-bot@chromium.org via codereview.chromium.org
Apr 9, 2016, 3:12:32 AM4/9/16
to gui...@chromium.org, joc...@chromium.org, har...@chromium.org, h...@chromium.org, philipj+...@opera.com, to...@chromium.org, wole...@chromium.org, xhw...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, f...@opera.com, gl...@chromium.org, h...@chromium.org, kinuko...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, silv...@chromium.org, tommyw+w...@chromium.org, vcar...@chromium.org
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1815033003/420001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1815033003/420001
https://codereview.chromium.org/1815033003/
https://chromium-cq-status.appspot.com/patch-status/1815033003/420001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1815033003/420001
https://codereview.chromium.org/1815033003/
commit-bot@chromium.org via codereview.chromium.org
Apr 9, 2016, 4:32:39 AM4/9/16
to gui...@chromium.org, joc...@chromium.org, har...@chromium.org, h...@chromium.org, philipj+...@opera.com, to...@chromium.org, wole...@chromium.org, xhw...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, f...@opera.com, gl...@chromium.org, h...@chromium.org, kinuko...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, silv...@chromium.org, tommyw+w...@chromium.org, vcar...@chromium.org
Patchset 22 (id:??) landed as
https://crrev.com/9bfe4e2f473b01d5039dd31c9b04cc8c78ff5c70
Cr-Commit-Position: refs/heads/master@{#386294}
https://codereview.chromium.org/1815033003/
https://crrev.com/9bfe4e2f473b01d5039dd31c9b04cc8c78ff5c70
Cr-Commit-Position: refs/heads/master@{#386294}
https://codereview.chromium.org/1815033003/
commit-bot@chromium.org via codereview.chromium.org
Apr 9, 2016, 4:32:42 AM4/9/16
to gui...@chromium.org, joc...@chromium.org, har...@chromium.org, h...@chromium.org, philipj+...@opera.com, to...@chromium.org, wole...@chromium.org, xhw...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, f...@opera.com, gl...@chromium.org, h...@chromium.org, kinuko...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, silv...@chromium.org, tommyw+w...@chromium.org, vcar...@chromium.org
sigb...@opera.com
Apr 12, 2016, 2:40:23 PM4/12/16
to gui...@chromium.org, joc...@chromium.org, har...@chromium.org, h...@chromium.org, philipj+...@opera.com, to...@chromium.org, wole...@chromium.org, xhw...@chromium.org, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, f...@opera.com, gl...@chromium.org, h...@chromium.org, kinuko...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, silv...@chromium.org, tommyw+w...@chromium.org, vcar...@chromium.org
https://codereview.chromium.org/1815033003/diff/420001/third_party/WebKit/Source/core/html/HTMLMediaElement.h
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):
https://codereview.chromium.org/1815033003/diff/420001/third_party/WebKit/Source/core/html/HTMLMediaElement.h#newcode495
third_party/WebKit/Source/core/html/HTMLMediaElement.h:495:
WebMediaPlayerSource m_srcObject;
This value object contains a WebMediaStream, which in turn contains a
WebPrivatePtr<MediaStreamDescriptor> (a persistent) -- have this been
verified not to hold on to too much? i.e., that there isn't a cycle back
to HTMLMediaElement somehow via that WebPrivatePtr<>.
https://codereview.chromium.org/1815033003/
gui...@chromium.org
Apr 12, 2016, 5:38:55 PM4/12/16
to joc...@chromium.org, har...@chromium.org, h...@chromium.org, philipj+...@opera.com, to...@chromium.org, wole...@chromium.org, xhw...@chromium.org, sigb...@opera.com, blink-...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dglazko...@chromium.org, eric.c...@apple.com, feature-me...@chromium.org, f...@opera.com, gl...@chromium.org, h...@chromium.org, kinuko...@chromium.org, mlamouri+w...@chromium.org, philipj+...@opera.com, silv...@chromium.org, tommyw+w...@chromium.org, vcar...@chromium.org
https://codereview.chromium.org/1815033003/diff/420001/third_party/WebKit/Source/core/html/HTMLMediaElement.h
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):
https://codereview.chromium.org/1815033003/diff/420001/third_party/WebKit/Source/core/html/HTMLMediaElement.h#newcode495
third_party/WebKit/Source/core/html/HTMLMediaElement.h:495:
WebMediaPlayerSource m_srcObject;
On 2016/04/12 18:40:22, sof wrote:
> This value object contains a WebMediaStream, which in turn contains a
> WebPrivatePtr<MediaStreamDescriptor> (a persistent) -- have this been
verified
> not to hold on to too much? i.e., that there isn't a cycle back to
> HTMLMediaElement somehow via that WebPrivatePtr<>.
> This value object contains a WebMediaStream, which in turn contains a
> WebPrivatePtr<MediaStreamDescriptor> (a persistent) -- have this been
verified
> not to hold on to too much? i.e., that there isn't a cycle back to
> HTMLMediaElement somehow via that WebPrivatePtr<>.
I don't think there are any cycles, but I will replace the persistent
reference in an upcoming CL to avoid future problems.
https://codereview.chromium.org/1815033003/
reference in an upcoming CL to avoid future problems.
https://codereview.chromium.org/1815033003/
Reply all
Reply to author
Forward
0 new messages