Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
545933002:
Implement HTMLMediaElement::srcObject. (Closed)
|
Created:
6 years, 3 months ago by perkj_chrome Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, oilpan-reviews, nessy, tommyw+watchlist_chromium.org, vcarbune.chromium, Guido Urdaneta, hbos_chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionImplement HTMLMediaElement::srcObject.
This is a suggestion for how to implement the srcObject attribute of HTMLMediaElement.
https://html.spec.whatwg.org/#htmlmediaelement
This cl only address the case when the srcObject is a MediaStream but the design should be applicable for MediaSource and File as well.
http://w3c.github.io/mediacapture-main/getusermedia.html#direct-assignment-to-media-elements
BUG=387740
Patch Set 1 #Patch Set 2 : New approach - with #includes that violate checkdeps rules #
Total comments: 4
Patch Set 3 : Rewrite where each MediaProvider module register a converter method. #
Total comments: 3
Patch Set 4 : Now uses Functional. #
Total comments: 34
Patch Set 5 : Rebase only #
Total comments: 2
Patch Set 6 : Addressed comments. #
Total comments: 21
Patch Set 7 : Addressed sof's comments. #
Total comments: 8
Patch Set 8 : Addressed sofs easy comments. #
Total comments: 6
Messages
Total messages: 55 (7 generated)
perkj@chromium.org changed reviewers: + acolwell@chromium.org, grunell@chromium.org, hta@chromium.org
Hej Here comes my first attempt on a blink cl ever. So please bare with me for all possible rookie mistakes. grunell, hta, can you take a first pass to help me with the basics? acolwell, are you comfortable with this simple approach as a starter or would you prefer refatoring HTMLMediaElement to use an interface as a srcObject and let that interface be implemented by MediaStream and MediaSource?
srcObject is now part of the HTML spec, so I think it should go in HTMLMediaElement.idl to match: http://www.whatwg.org/specs/web-apps/current-work/#htmlmediaelement (Added in http://html5.org/r/8716 to fix https://www.w3.org/Bugs/Public/show_bug.cgi?id=24684)
On 2014/09/05 22:41:54, philipj wrote: > srcObject is now part of the HTML spec, so I think it should go in > HTMLMediaElement.idl to match: > http://www.whatwg.org/specs/web-apps/current-work/#htmlmediaelement > > (Added in http://html5.org/r/8716 to fix > https://www.w3.org/Bugs/Public/show_bug.cgi?id=24684) It is my first blink patch- To my understanding unions for attributes are not implemented in blink idl. So, you would do something like how CanvasStyle is implemented with a custom binding in blink to fix that? But MediaStreams and MediaSource are implemented in Modules, and HTMLMediaElement is implemented in Core. So how can I create the custom binding needed for HTMLMediaElement.srcObject for MediaStreams and MediaSource if I add srcObect to HTMLMediaElement.idl ?
On 2014/09/08 08:11:21, perkj wrote: > On 2014/09/05 22:41:54, philipj wrote: > > srcObject is now part of the HTML spec, so I think it should go in > > HTMLMediaElement.idl to match: > > http://www.whatwg.org/specs/web-apps/current-work/#htmlmediaelement > > > > (Added in http://html5.org/r/8716 to fix > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=24684) > > It is my first blink patch- Welcome! > To my understanding unions for attributes are not implemented in blink idl. > So, you would do something like how CanvasStyle is implemented with a custom > binding in blink to fix that? Yes, unless you want to hack on the IDL code generator, a V8HTMLMediaElementCustom.cpp seems like the way to go. > But MediaStreams and MediaSource are implemented in Modules, and > HTMLMediaElement is implemented in Core. So how can I create the custom binding > needed for HTMLMediaElement.srcObject for MediaStreams and MediaSource if I add > srcObect to HTMLMediaElement.idl ? Oh, I didn't think about that. By making srcObject a MediaStream? you avoid the problem, but in order to support the typedef (MediaStream or MediaSource or Blob) MediaProvider type I think the problem will come back, regardless of which IDL file this all goes into. I have limited experience with mediasource and mediastream, but if we want to make HTMLMediaElement completely ignorant about either, it seems like we need a MediaProvider type that internally holds a MediaStream, MediaSource or Blob, and that is able to create a WebMediaPlayer for the internal object. I don't know how feasible that is, you'll notice that m_mediaSource is used a lot in HTMLMediaElement. Some cases look like they could be hidden behind WebMediaPlayer, but I'm some can't. To get things started, I would suggest trying to do "typedef MediaStream MediaProvider" and "attribute MediaProvider? srcObject" in HTMLMediaElement.idl and seeing what changes are needed to avoid depending on modules/mediastream/ from HTMLMediaElement.cpp. I think srcObject should also be runtime-enabled until fully supported, unless there's a hurry.
Thanks. I thought I should upload what I have worked on today and maybe you can give me hint if I am on the right track? It is work in progress but the layout test seems to work. I also manually played around a bit testing with MediaSource. RefPtrWillBeMember<HTMLMediaSource> m_mediaSource in HTMLMediaElemnt should not be confused with a MediaSource :-> They don't have much in common. https://codereview.chromium.org/545933002/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp (right): https://codereview.chromium.org/545933002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp:34: #include "bindings/modules/v8/V8MediaSource.h" big no no I guess, and check deps complains. So how do I implement srcObjectAttributeGetterCustom to avoid this but still make sure HTMLMediaElement::srcObject returns the correct type? Implement MediaProvider in modules and add srcObject as partial interface to the HTMLMediaElement in modules?
philipj@opera.com changed reviewers: + haraken@chromium.org
+haraken, who knows a lot more about what is and is not allowed in custom bindings.
haraken@chromium.org changed reviewers: + bashi@chromium.org
bashi-san: Do you have a comment about the custom bindings? It's about a union type in a return value. https://codereview.chromium.org/545933002/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp (right): https://codereview.chromium.org/545933002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp:51: static PassRefPtrWillBeRawPtr<MediaProvider> toMediaProvider(v8::Handle<v8::Value> value, v8::Isolate* isolate) Can you implement this logic inside HTMLMediaElement::setSrcObject so that we don't need custom bindings?
https://codereview.chromium.org/545933002/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp (right): https://codereview.chromium.org/545933002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp:2: * Copyright (C) 2014 Google Inc. All rights reserved. New code should use a shorter copyright header, see e.g. HTMLMenuItemElement.idl https://codereview.chromium.org/545933002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp:34: #include "bindings/modules/v8/V8MediaSource.h" On 2014/09/08 18:45:43, perkj wrote: > big no no I guess, and check deps complains. > So how do I implement srcObjectAttributeGetterCustom to avoid this but still > make sure HTMLMediaElement::srcObject returns the correct type? > > Implement MediaProvider in modules and add srcObject as partial interface to the > HTMLMediaElement in modules? I haven't had a similar situation before, but quite clearly it's not intended that Source/core/ include from Source/modules/. To break the dependency, I suppose one could have a system where modules/mediastream/ registers a converter class with core that is able to convert between e.g. MediaStream* and the V8 types. Similar to MediaStreamRegistry::MediaStreamRegistry(), which calls HTMLMediaElement::setMediaStreamRegistry(this).
On 2014/09/09 08:06:02, haraken wrote: > bashi-san: Do you have a comment about the custom bindings? It's about a union > type in a return value. As perkj and philipj said, we don't have union support for attributes so custom binding is the solution for now. I'm working on re-designing the implementation of union support to support attributes/return values, but it may take time. We may want to extend the current implementation to support attributes, but I guess it's not a trivial change.
On 2014/09/08 18:45:43, perkj wrote: > Implement MediaProvider in modules and add srcObject as partial interface to the > HTMLMediaElement in modules? I think, this is a good way to avoid violating checkdeps violation. If we cannot do this, we need to move mediastream from modules to core.
On 2014/09/10 03:12:26, tasak wrote:
> On 2014/09/08 18:45:43, perkj wrote:
>
> > Implement MediaProvider in modules and add srcObject as partial interface to
> the
> > HTMLMediaElement in modules?
>
> I think, this is a good way to avoid violating checkdeps violation.
^^^^^^^^^^^^^^^^^^^
I'm sorry. "checkdeps rules"
perkj@chromium.org changed reviewers: + jochen@chromium.org
On 2014/09/10 03:14:46, tasak wrote: > On 2014/09/10 03:12:26, tasak wrote: > > On 2014/09/08 18:45:43, perkj wrote: > > > > > Implement MediaProvider in modules and add srcObject as partial interface to > > the > > > HTMLMediaElement in modules? > > > > I think, this is a good way to avoid violating checkdeps violation. > ^^^^^^^^^^^^^^^^^^^ > I'm sorry. "checkdeps rules" Anyone against that I try implementing HTMLMediaElement::srcObject as a partial interface in modules?
Which module? This is a boundary-crossing concern that doesn't make sense in modules/mediastream/ or modules/mediasource/... so a new module? Did you investigate following the model of HTMLMediaElement::setMediaStreamRegistry()?
maybe we should have a thread on blink-dev@ to come to a solution how to handle this? it seems like if none of the media modules are enabled, it would be desirable to still support blob so maybe we'd need a way for the modules to subscribe to this property in some way, and the blob version being in core/?
On 2014/09/10 11:31:27, jochen wrote: > maybe we should have a thread on blink-dev@ to come to a solution how to handle > this? > > it seems like if none of the media modules are enabled, it would be desirable to > still support blob > > so maybe we'd need a way for the modules to subscribe to this property in some > way, and the blob version being in core/? I think that is what philipj means when writing following the model of HTMLMediaElement::setRegistry, right? philipj- I dont understand what you mean by following the model of HTMLMediaElement::setRegistry in this case. The problem in my cl is the conversion and typecheck in static PassRefPtrWillBeRawPtr<MediaProvider> toMediaProvider(v8::Handle<v8::Value> value, v8::Isolate* isolate) right? Should each model that wants to support MediaProvider register a converter function from v8::Handle<v8::Value> value, v8::Isolate* isolate) to a MediProvider in HTMLMediaElement ? Am I allowed to use v8 types outside of bindings? I then assume srcObjectAttributeSetterCustom(v8::Local<v8::Value> value, const v8::PropertyCallbackInfo<void>& info) should call into the HTMLMediElement with value and isolate and let the registry do the conversion? Or is it possible to do this in the bindings? Can I do this in V8HTMLMediaElement and V8MediaStream etc ? Each model can register a converter method that to a static registry in HTMLMediaElement. But wouldn't the argumens to that converter function be v8::Handle<v8::Value> value, v8::Isolate* isolate?
On 2014/09/10 14:33:33, perkj wrote: > On 2014/09/10 11:31:27, jochen wrote: > > maybe we should have a thread on blink-dev@ to come to a solution how to > handle > > this? > > > > it seems like if none of the media modules are enabled, it would be desirable > to > > still support blob > > > > so maybe we'd need a way for the modules to subscribe to this property in some > > way, and the blob version being in core/? > I think that is what philipj means when writing following the model of > HTMLMediaElement::setRegistry, right? > > > philipj- I dont understand what you mean by following the model of > HTMLMediaElement::setRegistry in this case. > The problem in my cl is the conversion and typecheck in static > PassRefPtrWillBeRawPtr<MediaProvider> toMediaProvider(v8::Handle<v8::Value> > value, v8::Isolate* isolate) right? If checkdeps is to be trusted, yes. > Should each model that wants to support MediaProvider register a converter > function from v8::Handle<v8::Value> value, v8::Isolate* isolate) to a > MediProvider in HTMLMediaElement ? > Am I allowed to use v8 types outside of bindings? There's also ScriptValue that's used for IDBRequest.source and in other places, I'm not sure if that could be of use. > I then assume srcObjectAttributeSetterCustom(v8::Local<v8::Value> value, const > v8::PropertyCallbackInfo<void>& info) should call into the HTMLMediElement with > value and isolate and let the registry do the conversion? > Or is it possible to do this in the bindings? Can I do this in > V8HTMLMediaElement and V8MediaStream etc ? Well not exactly, it doesn't necessarily have to be HTMLMediaElement which the modules register on, MediaProvider may be more natural. If one could arrange things such that HTMLMediaElement simply has a setSrcObject(MediaProvider&) and the MediaProvider object is later either used to create a WebMediaPlayer or passed to a WebMediaPlayer to start the load, that should allow HTMLMediaElement to know nothing about MediaStream. > Each model can register a converter method that to a static registry in > HTMLMediaElement. But wouldn't the argumens to that converter function be > v8::Handle<v8::Value> value, v8::Isolate* isolate? If Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp is allowed to use things in Source/bindings/modules/, then perhaps the v8 types can be kept in bindings alone. Or possibly ScriptValue is a part of the solution. So, perhaps a static MediaProvider::fromScriptValue(...) that tries the registered converters in turn for the setter, and a virtual MediaProvider::toScriptValue() for the getter? In truth, I don't know yet exactly what the solution is, but it needs to work for MediaSource and Blob without a full rewrite, and it mustn't anger checkdeps. If you think a partial interface in modules is viable, please do expand on that :)
On 2014/09/10 15:26:40, philipj wrote: > On 2014/09/10 14:33:33, perkj wrote: > > On 2014/09/10 11:31:27, jochen wrote: > > > maybe we should have a thread on blink-dev@ to come to a solution how to > > handle > > > this? > > > > > > it seems like if none of the media modules are enabled, it would be > desirable > > to > > > still support blob > > > > > > so maybe we'd need a way for the modules to subscribe to this property in > some > > > way, and the blob version being in core/? > > I think that is what philipj means when writing following the model of > > HTMLMediaElement::setRegistry, right? > > > > > > philipj- I dont understand what you mean by following the model of > > HTMLMediaElement::setRegistry in this case. > > The problem in my cl is the conversion and typecheck in static > > PassRefPtrWillBeRawPtr<MediaProvider> toMediaProvider(v8::Handle<v8::Value> > > value, v8::Isolate* isolate) right? > > If checkdeps is to be trusted, yes. > > > Should each model that wants to support MediaProvider register a converter > > function from v8::Handle<v8::Value> value, v8::Isolate* isolate) to a > > MediProvider in HTMLMediaElement ? > > Am I allowed to use v8 types outside of bindings? > > There's also ScriptValue that's used for IDBRequest.source and in other places, > I'm not sure if that could be of use. > > > I then assume srcObjectAttributeSetterCustom(v8::Local<v8::Value> value, const > > v8::PropertyCallbackInfo<void>& info) should call into the HTMLMediElement > with > > value and isolate and let the registry do the conversion? > > Or is it possible to do this in the bindings? Can I do this in > > V8HTMLMediaElement and V8MediaStream etc ? > > Well not exactly, it doesn't necessarily have to be HTMLMediaElement which the > modules register on, MediaProvider may be more natural. If one could arrange > things such that HTMLMediaElement simply has a setSrcObject(MediaProvider&) and > the MediaProvider object is later either used to create a WebMediaPlayer or > passed to a WebMediaPlayer to start the load, that should allow HTMLMediaElement > to know nothing about MediaStream. > > > Each model can register a converter method that to a static registry in > > HTMLMediaElement. But wouldn't the argumens to that converter function be > > v8::Handle<v8::Value> value, v8::Isolate* isolate? > > If Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp is allowed to use > things in Source/bindings/modules/, then perhaps the v8 types can be kept in > bindings alone. Or possibly ScriptValue is a part of the solution. > > So, perhaps a static MediaProvider::fromScriptValue(...) that tries the > registered converters in turn for the setter, and a virtual > MediaProvider::toScriptValue() for the getter? > > In truth, I don't know yet exactly what the solution is, but it needs to work > for MediaSource and Blob without a full rewrite, and it mustn't anger checkdeps. > If you think a partial interface in modules is viable, please do expand on that > :) Thanks - I get to learn (or at least look at) a lot of new concepts in my first cl. :-> It looks like the v8 namespace is allowed to be used from blink - I looked at IDB as you pointed out and expecically IDBKey* scriptValueToIDBKey(v8::Isolate*, const ScriptValue&);. So I will check if I can do a cl that uses a registry for converting from JS to a MediaProvider.
On 2014/09/10 at 16:38:16, perkj wrote: > On 2014/09/10 15:26:40, philipj wrote: > > On 2014/09/10 14:33:33, perkj wrote: > > > On 2014/09/10 11:31:27, jochen wrote: > > > > maybe we should have a thread on blink-dev@ to come to a solution how to > > > handle > > > > this? > > > > > > > > it seems like if none of the media modules are enabled, it would be > > desirable > > > to > > > > still support blob > > > > > > > > so maybe we'd need a way for the modules to subscribe to this property in > > some > > > > way, and the blob version being in core/? > > > I think that is what philipj means when writing following the model of > > > HTMLMediaElement::setRegistry, right? > > > > > > > > > philipj- I dont understand what you mean by following the model of > > > HTMLMediaElement::setRegistry in this case. > > > The problem in my cl is the conversion and typecheck in static > > > PassRefPtrWillBeRawPtr<MediaProvider> toMediaProvider(v8::Handle<v8::Value> > > > value, v8::Isolate* isolate) right? > > > > If checkdeps is to be trusted, yes. > > > > > Should each model that wants to support MediaProvider register a converter > > > function from v8::Handle<v8::Value> value, v8::Isolate* isolate) to a > > > MediProvider in HTMLMediaElement ? > > > Am I allowed to use v8 types outside of bindings? > > > > There's also ScriptValue that's used for IDBRequest.source and in other places, > > I'm not sure if that could be of use. > > > > > I then assume srcObjectAttributeSetterCustom(v8::Local<v8::Value> value, const > > > v8::PropertyCallbackInfo<void>& info) should call into the HTMLMediElement > > with > > > value and isolate and let the registry do the conversion? > > > Or is it possible to do this in the bindings? Can I do this in > > > V8HTMLMediaElement and V8MediaStream etc ? > > > > Well not exactly, it doesn't necessarily have to be HTMLMediaElement which the > > modules register on, MediaProvider may be more natural. If one could arrange > > things such that HTMLMediaElement simply has a setSrcObject(MediaProvider&) and > > the MediaProvider object is later either used to create a WebMediaPlayer or > > passed to a WebMediaPlayer to start the load, that should allow HTMLMediaElement > > to know nothing about MediaStream. > > > > > Each model can register a converter method that to a static registry in > > > HTMLMediaElement. But wouldn't the argumens to that converter function be > > > v8::Handle<v8::Value> value, v8::Isolate* isolate? > > > > If Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp is allowed to use > > things in Source/bindings/modules/, then perhaps the v8 types can be kept in > > bindings alone. Or possibly ScriptValue is a part of the solution. > > > > So, perhaps a static MediaProvider::fromScriptValue(...) that tries the > > registered converters in turn for the setter, and a virtual > > MediaProvider::toScriptValue() for the getter? > > > > In truth, I don't know yet exactly what the solution is, but it needs to work > > for MediaSource and Blob without a full rewrite, and it mustn't anger checkdeps. > > If you think a partial interface in modules is viable, please do expand on that > > :) > > Thanks - I get to learn (or at least look at) a lot of new concepts in my first cl. :-> > It looks like the v8 namespace is allowed to be used from blink - I looked at IDB as you pointed out and expecically IDBKey* scriptValueToIDBKey(v8::Isolate*, const ScriptValue&);. > So I will check if I can do a cl that uses a registry for converting from JS to a MediaProvider. we try to avoid accessing the V8 API outside of bindings other than passing around Isolate pointer We would usually introduce some wrapper class like ScriptValue for handles and use that instead.
Hej How about this approach? Thanks all for your input so far. This patch uses ScriptValues and is registering converters. The idea is that MediaStream, MediaSource and file will inherit the MediaProvider interface and register a converter function that takes a ScriptValue as an argument. https://codereview.chromium.org/545933002/diff/40001/Source/core/html/MediaPr... File Source/core/html/MediaProvider.h (right): https://codereview.chromium.org/545933002/diff/40001/Source/core/html/MediaPr... Source/core/html/MediaProvider.h:46: virtual MediaProvider* getMediaProvider(const ScriptValue&) = 0; Is there some type of function pointers in blink I can use instead MediProviderConverter? Ie, each module must register only one converter method so all I need is a registry of static converter functions. https://codereview.chromium.org/545933002/diff/40001/Source/platform/Logging.cpp File Source/platform/Logging.cpp (right): https://codereview.chromium.org/545933002/diff/40001/Source/platform/Logging.... Source/platform/Logging.cpp:59: WTFLogChannel LogMedia = { WTFLogChannelOn }; oops
https://codereview.chromium.org/545933002/diff/40001/Source/core/html/MediaPr... File Source/core/html/MediaProvider.h (right): https://codereview.chromium.org/545933002/diff/40001/Source/core/html/MediaPr... Source/core/html/MediaProvider.h:46: virtual MediaProvider* getMediaProvider(const ScriptValue&) = 0; On 2014/09/11 17:31:45, perkj wrote: > Is there some type of function pointers in blink I can use instead > MediProviderConverter? Ie, each module must register only one converter method > so all I need is a registry of static converter functions. I haven't looked at any of the code yet, but it sounds like Source/wtf/Functional.h may be relevant. For example Scheduler.h has a typedef Function<void(double allottedTimeMs)> IdleTask; It's not used that much in the code base, I'm not sure why that is.
acolwell@chromium.org changed reviewers: - acolwell@chromium.org
ok- Something like this then. https://codereview.chromium.org/545933002/diff/60001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/545933002/diff/60001/Source/core/core.gypi#ne... Source/core/core.gypi:220: 'html/ImageData.idl', todo fix... https://codereview.chromium.org/545933002/diff/60001/Source/modules/mediastre... File Source/modules/mediastream/HTMLMediaElementMediaStream.idl (right): https://codereview.chromium.org/545933002/diff/60001/Source/modules/mediastre... Source/modules/mediastream/HTMLMediaElementMediaStream.idl:32: // http://w3c.github.io/mediacapture-main/getusermedia.html#direct-assignment-to... todo remove this file. https://codereview.chromium.org/545933002/diff/60001/Source/modules/modules.gypi File Source/modules/modules.gypi (right): https://codereview.chromium.org/545933002/diff/60001/Source/modules/modules.g... Source/modules/modules.gypi:557: 'mediastream/MediaStreamEvent.h', fix
Sorry I haven't had time to review this today, I've had to prioritize https://codereview.chromium.org/552303006/ For this or any other review, if you get impatient, don't hesitate to ping me.
A not so loud ping?
In the description, please point to https://html.spec.whatwg.org/#htmlmediaelement instead. If you could also update the description to explain the new deal in high-level terms and wrap lines at 72 columns, that would be great. In the meantime, I'm looking at the actual code.
Could you also rebase again? I'd like to apply it locally to look around, but have trouble finding the point where it applied without conflicts.
Feedback on HTMLMediaElement.*. I haven't looked at anything else in detail yet, but this is a start for you. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:329: , m_srcObject(std::nullptr_t()) You don't need to explicitly init these smart pointer types. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:689: WTF_LOG(Media, "HTMLMediaElement::setSrcObject()"); Please include the %p this pointer as well. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:692: scheduleDelayedAction(LoadMediaResource); The spec says "On setting, it must set the element's assigned media provider object to the new value, and then invoke the element's media element load algorithm." That means simply calling load() here, or at least it would if load() were implemented per spec. It should be closer to spec than this though, please add a test that verifies that some of the reset steps of load() have run immediately after setting srcObject, e.g. test(function() { var v = document.createElement('video'); v.playbackRate = 2; v.srcObject = mediaStreamOrSomething; assert_equals(v.playbackRate, 1); }); https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:695: MediaProvider* HTMLMediaElement::srcObject() const Please invert order here too. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:875: // 3 - Decide which source to load. Please copy the steps from the spec. This should begin with "If the media element has an assigned media provider object, then let mode be object." https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:879: Mode mode = Object; Remove the default init unless it's needed to squash some compiler warning? It shouldn't be unless the compiler is very short-sighted. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:909: // 6 - Load the correct source. Should say "Run the appropriate steps from the following list:" https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:912: loadSourceFromObject(); I think just loadFromObject() and loadFromAttribute() would be better, it's not a source that's being loaded, but rather one may end up in the resource fetch algorithm, or abort early. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:925: m_loadState = LoadingFromSrcObject; Can you annotate this function with comments from the spec? In particular I see that "Set the currentSrc attribute to the empty string." does not seem to be implemented. That would also need a test. First get a video element into a state where currentSrc!="", then set srcObject and verify that immediately after currentSrc is "". https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:927: KURL mediaURL = document().completeURL(m_srcObject->getObjectUrl()); This is a little bit peculiar, since the point of srcObject is to avoid these magic URLs. I don't have a concrete proposal, but eventually I think it would be nice to unify things in the other direction, so that a magic URL is internally handled like a MediaProvider object, and that any special load works by creating a WebMediaPlayer from a MediaProver in some way. Anyway, what you have here will get things started, and I don't believe it really creates any additional complexity, so no action required. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:930: mediaLoadingFailed(WebMediaPlayer::NetworkStateFormatError); This bit needs a test. It can cause HTMLMediaElement::noneSupported() to run which will set m_error synchronously. Per spec the synchronous section has already ended here, so there's no way that the error could be modified synchronously in the srcObject setter. In order to pass that test, some code change would be needed too. Alternatively, if there actually is no case where isSafeToLoadURL() would return false here, assert that and save some trouble. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:934: // No type or key system information is available when the url comes This is repeated, can you add a shared void loadResource(const KURL&) override that provides the empty ContentType and keySystem instead? https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:119: void setSrcObject(RawPtr<MediaProvider>); Can this simply be MediaProvider*, since that's the type at the call site and internally it isn't RawPtr<> anyway? Also, please switch the order of these to match other getter/setter pairs. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.idl (right): https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.idl:28: typedef (MediaStream or MediaSource or Blob) MediaProvider; Remove these bits since they are unused. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.idl:42: // FIXME: should be attribute MediaProvider? srcObject;　ｈttp://crbug.com/240176 There's a suspicious non-ASCII "ｈ" here.
https://codereview.chromium.org/545933002/diff/80001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/545933002/diff/80001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:868: enum Mode {Object, Attribute, Children}; Please keep the whitespace after/before {/}, all the enums in the header file do that.
Thanks for your review. I addressed your comments and hope I did it correct. https://codereview.chromium.org/545933002/diff/60001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/545933002/diff/60001/Source/core/core.gypi#ne... Source/core/core.gypi:220: 'html/ImageData.idl', On 2014/09/12 08:22:19, perkj wrote: > todo fix... Done. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:329: , m_srcObject(std::nullptr_t()) On 2014/09/16 13:39:50, philipj wrote: > You don't need to explicitly init these smart pointer types. Well, WTF::RawPtr does not allow you to not set a pointer value and later check for null. But there is an explicit constructur that seems to be allowed RawPtr(std::nullptr_t). https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:689: WTF_LOG(Media, "HTMLMediaElement::setSrcObject()"); On 2014/09/16 13:39:50, philipj wrote: > Please include the %p this pointer as well. Done. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:692: scheduleDelayedAction(LoadMediaResource); On 2014/09/16 13:39:50, philipj wrote: > The spec says "On setting, it must set the element's assigned media provider > object to the new value, and then invoke the element's media element load > algorithm." > > That means simply calling load() here, or at least it would if load() were > implemented per spec. It should be closer to spec than this though, please add a > test that verifies that some of the reset steps of load() have run immediately > after setting srcObject, e.g. > > test(function() > { > var v = document.createElement('video'); > v.playbackRate = 2; > v.srcObject = mediaStreamOrSomething; > assert_equals(v.playbackRate, 1); > }); Now calls load() and test is added to mediastream-srcobject.html https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:695: MediaProvider* HTMLMediaElement::srcObject() const On 2014/09/16 13:39:50, philipj wrote: > Please invert order here too. Done. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:875: // 3 - Decide which source to load. On 2014/09/16 13:39:51, philipj wrote: > Please copy the steps from the spec. This should begin with "If the media > element has an assigned media provider object, then let mode be object." Done. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:879: Mode mode = Object; On 2014/09/16 13:39:51, philipj wrote: > Remove the default init unless it's needed to squash some compiler warning? It > shouldn't be unless the compiler is very short-sighted. I can try. I am afraid it will hit me on one platform. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:909: // 6 - Load the correct source. On 2014/09/16 13:39:50, philipj wrote: > Should say "Run the appropriate steps from the following list:" Done. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:912: loadSourceFromObject(); On 2014/09/16 13:39:50, philipj wrote: > I think just loadFromObject() and loadFromAttribute() would be better, it's not > a source that's being loaded, but rather one may end up in the resource fetch > algorithm, or abort early. Done. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:925: m_loadState = LoadingFromSrcObject; On 2014/09/16 13:39:50, philipj wrote: > Can you annotate this function with comments from the spec? In particular I see > that "Set the currentSrc attribute to the empty string." does not seem to be > implemented. That would also need a test. First get a video element into a state > where currentSrc!="", then set srcObject and verify that immediately after > currentSrc is "". Added test of currentSrc to mediastream-srcobject.html. The implementation is a bit hacky due to how loadResource and both MediaStreams and MediaSource require the use of URLs. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:927: KURL mediaURL = document().completeURL(m_srcObject->getObjectUrl()); On 2014/09/16 13:39:51, philipj wrote: > This is a little bit peculiar, since the point of srcObject is to avoid these > magic URLs. I don't have a concrete proposal, but eventually I think it would be > nice to unify things in the other direction, so that a magic URL is internally > handled like a MediaProvider object, and that any special load works by creating > a WebMediaPlayer from a MediaProver in some way. > > Anyway, what you have here will get things started, and I don't believe it > really creates any additional complexity, so no action required. Agree, this cl adds the interface MediaProvider so it should be possible. But switching all cases to use a new interface sounds like a huge task since there are many objects that today uses the src url. There are also two different media player implementations in Chrome. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:930: mediaLoadingFailed(WebMediaPlayer::NetworkStateFormatError); On 2014/09/16 13:39:51, philipj wrote: > This bit needs a test. It can cause HTMLMediaElement::noneSupported() to run > which will set m_error synchronously. Per spec the synchronous section has > already ended here, so there's no way that the error could be modified > synchronously in the srcObject setter. In order to pass that test, some code > change would be needed too. > > Alternatively, if there actually is no case where isSafeToLoadURL() would return > false here, assert that and save some trouble. I don't see how it would ever return anything but true since the url is generated internally by blink. But I can't say that I understand exactly what it it that it checks. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:934: // No type or key system information is available when the url comes On 2014/09/16 13:39:51, philipj wrote: > This is repeated, can you add a shared void loadResource(const KURL&) override > that provides the empty ContentType and keySystem instead? Done. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:119: void setSrcObject(RawPtr<MediaProvider>); On 2014/09/16 13:39:51, philipj wrote: > Can this simply be MediaProvider*, since that's the type at the call site and > internally it isn't RawPtr<> anyway? > > Also, please switch the order of these to match other getter/setter pairs. Done. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.idl (right): https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.idl:28: typedef (MediaStream or MediaSource or Blob) MediaProvider; On 2014/09/16 13:39:51, philipj wrote: > Remove these bits since they are unused. Removed but merged the typedef comment with the fixme below. https://codereview.chromium.org/545933002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.idl:42: // FIXME: should be attribute MediaProvider? srcObject;　ｈttp://crbug.com/240176 On 2014/09/16 13:39:51, philipj wrote: > There's a suspicious non-ASCII "ｈ" here. Done. https://codereview.chromium.org/545933002/diff/80001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/545933002/diff/80001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:868: enum Mode {Object, Attribute, Children}; On 2014/09/17 09:25:39, philipj wrote: > Please keep the whitespace after/before {/}, all the enums in the header file do > that. Done.
Some Oilpan-focused feedback. https://codereview.chromium.org/545933002/diff/100001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/545933002/diff/100001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:329: , m_srcObject(std::nullptr_t()) m_srcObject(nullptr) https://codereview.chromium.org/545933002/diff/100001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:978: ContentType contentType((String())); redundant () https://codereview.chromium.org/545933002/diff/100001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:3983: visitor->trace(m_currentSourceNode); Fields that refer to objects that are on the Oilpan heap needs to be traced during a garbage collection, so as to keep them alive. So, whenever you add a Member<T> (via the transition type RawPtrWillBeMember<T> right now), you need to extend trace() accordingly. So, add visitor->trace(m_srvObject); right here. https://codereview.chromium.org/545933002/diff/100001/Source/core/html/MediaP... File Source/core/html/MediaProvider.cpp (right): https://codereview.chromium.org/545933002/diff/100001/Source/core/html/MediaP... Source/core/html/MediaProvider.cpp:2: * Copyright (C) 2014 Google Inc. All rights reserved. New files are preferably adorned with a shorter version of this one, coul you replace it with the 4-line version? (cf. DOMWindowMediaStream.h in this directory.) https://codereview.chromium.org/545933002/diff/100001/Source/core/html/MediaP... File Source/core/html/MediaProvider.h (right): https://codereview.chromium.org/545933002/diff/100001/Source/core/html/MediaP... Source/core/html/MediaProvider.h:2: * Copyright (C) 2014 Google Inc. All rights reserved. Use a short copyright here also. https://codereview.chromium.org/545933002/diff/100001/Source/core/html/MediaP... Source/core/html/MediaProvider.h:43: class MediaProvider : public WillBeGarbageCollectedMixin { Do you need to derive from WillBeGarbageCollectedMixin ? If so, add a public virtual method virtual void trace(Visitor*) { } and delegate to from the trace() method from any object that derives from MediaProvider. (For consistency, we prefer to define a trace() for all mixins.) https://codereview.chromium.org/545933002/diff/100001/Source/core/html/MediaP... Source/core/html/MediaProvider.h:51: virtual ~MediaProvider() { }; nit: remove ";" https://codereview.chromium.org/545933002/diff/100001/Source/modules/mediastr... File Source/modules/mediastream/MediaStream.cpp (right): https://codereview.chromium.org/545933002/diff/100001/Source/modules/mediastr... Source/modules/mediastream/MediaStream.cpp:63: MediaStream* stream = V8MediaStream::toImplWithTypeCheck(value.isolate(), v8Value); Just return this directly -- return V8MediaStream::toImplWithTypeCheck(value.isolate(), v8Value); https://codereview.chromium.org/545933002/diff/100001/Source/modules/mediastr... Source/modules/mediastream/MediaStream.cpp:409: m_objectUrl = m_objectUrl = DOMURL::createPublicURL(executionContext(), this); "m_objectUrl = m_objectUrl" => "m_objectUrl" ? https://codereview.chromium.org/545933002/diff/100001/Source/modules/mediastr... Source/modules/mediastream/MediaStream.cpp:409: m_objectUrl = m_objectUrl = DOMURL::createPublicURL(executionContext(), this); Should this be using URLMediaStream::createObjectURL() ?
Thank you for your feedback. Do you think this is an ok solution? jochen, is this solution ok with you or should I still ask on the blink-list? https://codereview.chromium.org/545933002/diff/100001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/545933002/diff/100001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:329: , m_srcObject(std::nullptr_t()) On 2014/09/18 07:33:17, sof wrote: > m_srcObject(nullptr) Done. https://codereview.chromium.org/545933002/diff/100001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:978: ContentType contentType((String())); On 2014/09/18 07:33:17, sof wrote: > redundant () This does not compile without the extra parenthesis. ContentType(const String& contentType) Also see loadNextSourceChild(). https://codereview.chromium.org/545933002/diff/100001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:3983: visitor->trace(m_currentSourceNode); On 2014/09/18 07:33:17, sof wrote: > Fields that refer to objects that are on the Oilpan heap needs to be traced > during a garbage collection, so as to keep them alive. So, whenever you add a > Member<T> (via the transition type RawPtrWillBeMember<T> right now), you need to > extend trace() accordingly. So, add > > visitor->trace(m_srvObject); > > right here. oops, right- thanks. https://codereview.chromium.org/545933002/diff/100001/Source/core/html/MediaP... File Source/core/html/MediaProvider.cpp (right): https://codereview.chromium.org/545933002/diff/100001/Source/core/html/MediaP... Source/core/html/MediaProvider.cpp:2: * Copyright (C) 2014 Google Inc. All rights reserved. On 2014/09/18 07:33:17, sof wrote: > New files are preferably adorned with a shorter version of this one, coul you > replace it with the 4-line version? (cf. DOMWindowMediaStream.h in this > directory.) Done. https://codereview.chromium.org/545933002/diff/100001/Source/core/html/MediaP... File Source/core/html/MediaProvider.h (right): https://codereview.chromium.org/545933002/diff/100001/Source/core/html/MediaP... Source/core/html/MediaProvider.h:2: * Copyright (C) 2014 Google Inc. All rights reserved. On 2014/09/18 07:33:17, sof wrote: > Use a short copyright here also. Done. https://codereview.chromium.org/545933002/diff/100001/Source/core/html/MediaP... Source/core/html/MediaProvider.h:43: class MediaProvider : public WillBeGarbageCollectedMixin { On 2014/09/18 07:33:17, sof wrote: > Do you need to derive from WillBeGarbageCollectedMixin ? If so, add a public > virtual method > > virtual void trace(Visitor*) { } > > and delegate to from the trace() method from any object that derives from > MediaProvider. > > (For consistency, we prefer to define a trace() for all mixins.) I do right? Since HTMLMediaElement must be able to keep a MediaStream alive if its set as a srcObject. https://codereview.chromium.org/545933002/diff/100001/Source/core/html/MediaP... Source/core/html/MediaProvider.h:51: virtual ~MediaProvider() { }; On 2014/09/18 07:33:17, sof wrote: > nit: remove ";" Done. https://codereview.chromium.org/545933002/diff/100001/Source/modules/mediastr... File Source/modules/mediastream/MediaStream.cpp (right): https://codereview.chromium.org/545933002/diff/100001/Source/modules/mediastr... Source/modules/mediastream/MediaStream.cpp:63: MediaStream* stream = V8MediaStream::toImplWithTypeCheck(value.isolate(), v8Value); On 2014/09/18 07:33:17, sof wrote: > Just return this directly -- > > return V8MediaStream::toImplWithTypeCheck(value.isolate(), v8Value); Done. https://codereview.chromium.org/545933002/diff/100001/Source/modules/mediastr... Source/modules/mediastream/MediaStream.cpp:409: m_objectUrl = m_objectUrl = DOMURL::createPublicURL(executionContext(), this); On 2014/09/18 07:33:17, sof wrote: > "m_objectUrl = m_objectUrl" => "m_objectUrl" ? good question... Sorry. https://codereview.chromium.org/545933002/diff/100001/Source/modules/mediastr... Source/modules/mediastream/MediaStream.cpp:409: m_objectUrl = m_objectUrl = DOMURL::createPublicURL(executionContext(), this); On 2014/09/18 07:33:17, sof wrote: > Should this be using URLMediaStream::createObjectURL() ? It can- but since I use DOMURL::revokeObjectURL I think this is more clear.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/545933002/diff/100001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/545933002/diff/100001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:978: ContentType contentType((String())); On 2014/09/18 14:47:08, perkj wrote: > On 2014/09/18 07:33:17, sof wrote: > > redundant () > This does not compile without the extra parenthesis. > ContentType(const String& contentType) > > Also see loadNextSourceChild(). > Oh, I see. Why not use emptyString? https://codereview.chromium.org/545933002/diff/120001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/545933002/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.h:517: RawPtrWillBeMember<MediaProvider> m_srcObject; Could you explain what keeps m_srcObject alive&valid in a non-Oilpan setting? If it is held onto by someone else & guaranteed to live beyond the media element, then it would be preferable to make that clear in a comment. https://codereview.chromium.org/545933002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/MediaStream.cpp (right): https://codereview.chromium.org/545933002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/MediaStream.cpp:63: MediaStream* stream = V8MediaStream::toImplWithTypeCheck(value.isolate(), v8Value); return V8MediaStream::toImplWithTypeCheck(value.isolate(), v8Value); https://codereview.chromium.org/545933002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/MediaStream.cpp:163: DOMURL::revokeObjectURL(executionContext(), m_objectUrl); (It'd be nice to have an auto-revocation public URL abstraction that handled this on destruction.)
As I said, I am a blink newbie so I am not confident when it comes to how all blink smart pointer and macros works and when to use refcounted base classes and when I should only use the OILPAN versions. https://codereview.chromium.org/545933002/diff/120001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/545933002/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.h:517: RawPtrWillBeMember<MediaProvider> m_srcObject; On 2014/09/19 06:26:23, sof wrote: > Could you explain what keeps m_srcObject alive&valid in a non-Oilpan setting? If > it is held onto by someone else & guaranteed to live beyond the media element, > then it would be preferable to make that clear in a comment. oh, good catch. This is probably not correct Is there a generic solution or do I have to make the same solution used by EventTarget.h ? That defines ref and deref and use DEFINE_EVENT_TARGET_REFCOUNTING? Can I disable OILPAN for MediaStreams and test? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... I was trying to understand how/if this works without OILPAN today if createObjectURL is used used to register a MediaStream and the last reference from JS to the MediaStream is abandonded. It looks like the MediaStream object itself would not survive? https://codereview.chromium.org/545933002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/MediaStream.cpp (right): https://codereview.chromium.org/545933002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/MediaStream.cpp:63: MediaStream* stream = V8MediaStream::toImplWithTypeCheck(value.isolate(), v8Value); On 2014/09/19 06:26:23, sof wrote: > return V8MediaStream::toImplWithTypeCheck(value.isolate(), v8Value); Done. https://codereview.chromium.org/545933002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/MediaStream.cpp:163: DOMURL::revokeObjectURL(executionContext(), m_objectUrl); On 2014/09/19 06:26:23, sof wrote: > (It'd be nice to have an auto-revocation public URL abstraction that handled > this on destruction.) Sure, Would that make sence even if we refactor MediaProvider to not use urls later? Is there another use case as well?
https://codereview.chromium.org/545933002/diff/120001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/545933002/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.h:517: RawPtrWillBeMember<MediaProvider> m_srcObject; On 2014/09/19 10:48:06, perkj wrote: > On 2014/09/19 06:26:23, sof wrote: > > Could you explain what keeps m_srcObject alive&valid in a non-Oilpan setting? > If > > it is held onto by someone else & guaranteed to live beyond the media element, > > then it would be preferable to make that clear in a comment. > > oh, good catch. This is probably not correct > Is there a generic solution or do I have to make the same solution used by > EventTarget.h ? That defines ref and deref and use > DEFINE_EVENT_TARGET_REFCOUNTING? > Can I disable OILPAN for MediaStreams and test? > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > Yes, exactly - ref-forwarding is how this is usually addressed. A better example might be DOMSettableTokenList here, i.e., you add a ref+deref pure virtual pair to MediaProvider #if !ENABLE(OILPAN) virtual void ref() = 0; virtual void deref() = 0; #endif and then provide impls on the objects implementing MediaProvider -- i.e., on MediaSource: #if !ENABLE(OILPAN) virtual void ref() OVERRIDE { RefCountedGarbageCollected<MediaStream>::ref(); } virtual void deref() OVERRIDE { RefCountedGarbageCollected<MediaStream>::deref(); } #endif As Oilpan is disabled by default, you don't have to do anything to compile&test. > I was trying to understand how/if this works without OILPAN today if > createObjectURL is used used to register a MediaStream and the last reference > from JS to the MediaStream is abandonded. It looks like the MediaStream object > itself would not survive? It didn't look as if it would survie. Unless there are cycles to be concerned with between the MediaProvider implementations and HTMLMediaElement, the latter should keep a ref ptr to its srcObject, non Oilpan. It's simpler and more regular with Oilpan, and the handling here looks correct to me. https://codereview.chromium.org/545933002/diff/120001/Source/modules/mediastr... File Source/modules/mediastream/MediaStream.cpp (right): https://codereview.chromium.org/545933002/diff/120001/Source/modules/mediastr... Source/modules/mediastream/MediaStream.cpp:163: DOMURL::revokeObjectURL(executionContext(), m_objectUrl); On 2014/09/19 10:48:06, perkj wrote: > On 2014/09/19 06:26:23, sof wrote: > > (It'd be nice to have an auto-revocation public URL abstraction that handled > > this on destruction.) > Sure, > Would that make sence even if we refactor MediaProvider to not use urls later? > Is there another use case as well? Certainly, this wasn't meant as an actionable issue for this CL, just thinking of ways to avoid having to remember to perform the revocation step.
I'm a bit behind the discussion on this. Sorry if the following comment doesn't make much sense. https://codereview.chromium.org/545933002/diff/140001/Source/bindings/core/v8... File Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp (right): https://codereview.chromium.org/545933002/diff/140001/Source/bindings/core/v8... Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp:31: ScriptValue scriptValue(ScriptState::current(info.GetIsolate()), value); I'm sorry for the back & forth, but I begin to think that the original approach (something like the patch set 2) looks simpler. It looks a bit redundant (and complicated) to convert the |value| to ScriptValue and then have the core/ do the value conversion using binding methods. It looks simpler just to do all the conversion here in custom bindings like this: if (V8Blob::hasInstance(value)) { ...; } else if (V8MediaStream::hasInstance(value)) { ...; } else if (...) { ...; } https://codereview.chromium.org/545933002/diff/140001/Source/core/html/MediaP... File Source/core/html/MediaProvider.cpp (right): https://codereview.chromium.org/545933002/diff/140001/Source/core/html/MediaP... Source/core/html/MediaProvider.cpp:51: container.registerMediaProviderConverter(interfaceName, converter); Don't we need to unregister the convert at some point (e.g., when the MediaProvider gets destructed)? https://codereview.chromium.org/545933002/diff/140001/Source/modules/mediastr... File Source/modules/mediastream/MediaStream.cpp (right): https://codereview.chromium.org/545933002/diff/140001/Source/modules/mediastr... Source/modules/mediastream/MediaStream.cpp:63: return V8MediaStream::toImplWithTypeCheck(value.isolate(), v8Value); Hmm, I'm not really happy with writing this kind of binding logic in core/. Can we move the conversion code into V8HTMLMediaElementCustom.cpp?
haraken, the problem with patchset 2 is the fact that MediaStream and MediSource are modules while HTMLMediaElement and File are in core. So the purpose of the registration mechanism proposed by philipj is to break the dependency from core to modules. Do you have another idea how to do that? https://codereview.chromium.org/545933002/diff/140001/Source/bindings/core/v8... File Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp (right): https://codereview.chromium.org/545933002/diff/140001/Source/bindings/core/v8... Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp:31: ScriptValue scriptValue(ScriptState::current(info.GetIsolate()), value); On 2014/09/20 08:52:20, haraken wrote: > > I'm sorry for the back & forth, but I begin to think that the original approach > (something like the patch set 2) looks simpler. > > It looks a bit redundant (and complicated) to convert the |value| to ScriptValue > and then have the core/ do the value conversion using binding methods. It looks > simpler just to do all the conversion here in custom bindings like this: > > if (V8Blob::hasInstance(value)) { > ...; > } else if (V8MediaStream::hasInstance(value)) { > ...; > } else if (...) { > ...; > } But that would require this to include bindings/modules/v8/V8MediaStream.h which I guess is not allowed since this file is in core? Do you have a suggestion for how to avoid that? https://codereview.chromium.org/545933002/diff/140001/Source/core/html/MediaP... File Source/core/html/MediaProvider.cpp (right): https://codereview.chromium.org/545933002/diff/140001/Source/core/html/MediaP... Source/core/html/MediaProvider.cpp:51: container.registerMediaProviderConverter(interfaceName, converter); On 2014/09/20 08:52:20, haraken wrote: > > Don't we need to unregister the convert at some point (e.g., when the > MediaProvider gets destructed)? I don't think so since what we register is the static converter to be able to convert all instances of MediaStreams to a MediaProviders. The purpose of this is to break the dependency from core to modules/mediastream https://codereview.chromium.org/545933002/diff/140001/Source/modules/mediastr... File Source/modules/mediastream/MediaStream.cpp (right): https://codereview.chromium.org/545933002/diff/140001/Source/modules/mediastr... Source/modules/mediastream/MediaStream.cpp:63: return V8MediaStream::toImplWithTypeCheck(value.isolate(), v8Value); On 2014/09/20 08:52:20, haraken wrote: > > Hmm, I'm not really happy with writing this kind of binding logic in core/. Can > we move the conversion code into V8HTMLMediaElementCustom.cpp? But can bindings/core/v8/V8HTMLMediaElementCustom really depend on "bindings/modules/v8/V8MediaStream.h" ?
On 2014/09/24 11:39:39, perkj wrote: > haraken, the problem with patchset 2 is the fact that > MediaStream and MediSource are modules while HTMLMediaElement and File are in > core. So the purpose of the registration mechanism proposed by philipj is to > break the dependency from core to modules. > Do you have another idea how to do that? > > https://codereview.chromium.org/545933002/diff/140001/Source/bindings/core/v8... > File Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp (right): > > https://codereview.chromium.org/545933002/diff/140001/Source/bindings/core/v8... > Source/bindings/core/v8/custom/V8HTMLMediaElementCustom.cpp:31: ScriptValue > scriptValue(ScriptState::current(info.GetIsolate()), value); > On 2014/09/20 08:52:20, haraken wrote: > > > > I'm sorry for the back & forth, but I begin to think that the original > approach > > (something like the patch set 2) looks simpler. > > > > It looks a bit redundant (and complicated) to convert the |value| to > ScriptValue > > and then have the core/ do the value conversion using binding methods. It > looks > > simpler just to do all the conversion here in custom bindings like this: > > > > if (V8Blob::hasInstance(value)) { > > ...; > > } else if (V8MediaStream::hasInstance(value)) { > > ...; > > } else if (...) { > > ...; > > } > > But that would require this to include bindings/modules/v8/V8MediaStream.h which > I guess is not allowed since this file is in core? Do you have a suggestion for > how to avoid that? > > https://codereview.chromium.org/545933002/diff/140001/Source/core/html/MediaP... > File Source/core/html/MediaProvider.cpp (right): > > https://codereview.chromium.org/545933002/diff/140001/Source/core/html/MediaP... > Source/core/html/MediaProvider.cpp:51: > container.registerMediaProviderConverter(interfaceName, converter); > On 2014/09/20 08:52:20, haraken wrote: > > > > Don't we need to unregister the convert at some point (e.g., when the > > MediaProvider gets destructed)? > > I don't think so since what we register is the static converter to be able to > convert all instances of MediaStreams to a MediaProviders. > The purpose of this is to break the dependency from core to modules/mediastream > > https://codereview.chromium.org/545933002/diff/140001/Source/modules/mediastr... > File Source/modules/mediastream/MediaStream.cpp (right): > > https://codereview.chromium.org/545933002/diff/140001/Source/modules/mediastr... > Source/modules/mediastream/MediaStream.cpp:63: return > V8MediaStream::toImplWithTypeCheck(value.isolate(), v8Value); > On 2014/09/20 08:52:20, haraken wrote: > > > > Hmm, I'm not really happy with writing this kind of binding logic in core/. > Can > > we move the conversion code into V8HTMLMediaElementCustom.cpp? > > But can bindings/core/v8/V8HTMLMediaElementCustom > really depend on "bindings/modules/v8/V8MediaStream.h" ? Thanks perkj, now I understand the issue :) tasak@ is an expert of the core-module componentization, so let me talk with him tomorrow.
So I would like to confirm: do you have any reason why MediaProvider.{h,cpp} is
placed under Souce/core/html?
On 2014/09/24 13:48:04, haraken wrote:
> Thanks perkj, now I understand the issue :) tasak@ is an expert of the
> core-module componentization, so let me talk with him tomorrow.
I think, if you have no strong reason, how about adding
HTMLMediaElementMediaStream.idl:
typedef (MediaStream or MediaSource or Blob) MediaProvider;
[
RuntimeEnabled=MediaStream,
] partial interface HTMLMediaElement {
attribute MediaProvider? srcObject;
};
and adding MediaProvider.{h,cpp} to Source/modules/MediaProvider/ (or
Source/modules/MediaStream) ?
On 2014/09/25 09:38:47, tasak wrote:
> So I would like to confirm: do you have any reason why MediaProvider.{h,cpp}
is
> placed under Souce/core/html?
>
> On 2014/09/24 13:48:04, haraken wrote:
>
> > Thanks perkj, now I understand the issue :) tasak@ is an expert of the
> > core-module componentization, so let me talk with him tomorrow.
>
> I think, if you have no strong reason, how about adding
> HTMLMediaElementMediaStream.idl:
>
> typedef (MediaStream or MediaSource or Blob) MediaProvider;
>
> [
> RuntimeEnabled=MediaStream,
> ] partial interface HTMLMediaElement {
> attribute MediaProvider? srcObject;
> };
>
> and adding MediaProvider.{h,cpp} to Source/modules/MediaProvider/ (or
> Source/modules/MediaStream) ?
I guess that should be possible as well. We would still have to have a
MediaProvider interface in core that MediaProvider can inherit from since we
need to use it in HTMLMediaElement which is in core. Also blob is implemented in
core.
phillipj - is that acceptable to you? To add a partial interface for this?
Is the idea a new mediaprovider module, or where would a HTMLMediaElementMediaStream.idl live? (Should that be HTMLMediaElementMediaProvider?) It's the same problem as in comment #17, since srcObject isn't specific to MediaStream or MediaSource, there's no module where it belongs. (AFAICT modules don't freely include from each other, I assume this is by design and would get in the way of putting any shared code into a module. I'm not very attached to the registry idea, but I think in order to get the dependencies right there does need to be a MediaProvider concept in core and a way for modules (and Blob) to act like a MediaProvider. (Somewhat on topic, because of Blob, I don't think requiring MediaProvider to be subclassed is great, but I haven't looked into an alternative.)
On 2014/09/25 14:25:21, philipj wrote: > Is the idea a new mediaprovider module, or where would a > HTMLMediaElementMediaStream.idl live? (Should that be > HTMLMediaElementMediaProvider?) It's the same problem as in comment #17, since > srcObject isn't specific to MediaStream or MediaSource, there's no module where > it belongs. (AFAICT modules don't freely include from each other, I assume this > is by design and would get in the way of putting any shared code into a module. > > I'm not very attached to the registry idea, but I think in order to get the > dependencies right there does need to be a MediaProvider concept in core and a > way for modules (and Blob) to act like a MediaProvider. (Somewhat on topic, > because of Blob, I don't think requiring MediaProvider to be subclassed is > great, but I haven't looked into an alternative.) I think, if we need to fix crossing modules, make modules/MediaProvider, move modules/MediaStream to modules/MediaProvider/MediaStream, and move modules/MediaSource to modules/MediaProvider/MediaSource. However, crossing core and modules boundary is worse than crossing modules. So we should not add core-modules crossing issue to avoid module-module crossing issue.
On 2014/09/29 07:46:53, tasak wrote: > On 2014/09/25 14:25:21, philipj wrote: > > Is the idea a new mediaprovider module, or where would a > > HTMLMediaElementMediaStream.idl live? (Should that be > > HTMLMediaElementMediaProvider?) It's the same problem as in comment #17, since > > srcObject isn't specific to MediaStream or MediaSource, there's no module > where > > it belongs. (AFAICT modules don't freely include from each other, I assume > this > > is by design and would get in the way of putting any shared code into a > module. > > > > I'm not very attached to the registry idea, but I think in order to get the > > dependencies right there does need to be a MediaProvider concept in core and a > > way for modules (and Blob) to act like a MediaProvider. (Somewhat on topic, > > because of Blob, I don't think requiring MediaProvider to be subclassed is > > great, but I haven't looked into an alternative.) > > I think, if we need to fix crossing modules, make modules/MediaProvider, move > modules/MediaStream to modules/MediaProvider/MediaStream, and move > modules/MediaSource to modules/MediaProvider/MediaSource. > > However, crossing core and modules boundary is worse than crossing modules. So > we should not add core-modules crossing issue to avoid module-module crossing > issue. Yeah, probably this issue is worth discussing in blink-dev@.
I think we should just move the media stuff to core/
On 2014/09/30 08:16:39, jochen (slow) wrote: > I think we should just move the media stuff to core/ This architectural decision seems to have been left dangling for the last 5 months as priority has been given to other projects. Is there a better basis for deciding this now than we had back then?
Kentaro just sent around a proposal for how to go forward with modules, maybe this question should be discussed in that context.
This is the thread in question: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/5np2p7gTBS4
Peeking at the proposed architecture, it seems that the relevant bit does not change: modules can depend on core, but core cannot depend on modules, i.e. HTMLMediaElement in core/ cannot know about MediaStream and MediaSource in modules/. tasak@, has anything happened with the ideas for managing dependencies from core to modules?
Per, can this be closed?
perkj@chromium.org changed reviewers: - bashi@chromium.org, grunell@chromium.org, haraken@chromium.org, jochen@chromium.org
adding hbos and guidou for context - we'll have to come back to this one Real Soon. |
