Bug 121943

Summary: [MediaStream API] HTMLMediaElement should be able to use MediaStream as source
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: WebCore Misc.Assignee: Nick Diego Yamane (diegoyam) <nick.diego>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dino, esprehn+autocc, glenn, gtk-ews, gyuyoung.kim, hta, jer.noble, kaisers, kondapallykalyan, lauro.neto, nick.diego, rakuco, sergio, thiago.lacerda, tommyw, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 124856, 124860, 124896    
Bug Blocks: 124288    
Attachments:
Description Flags
Patch
none
Added Xcode stuff
none
Proposed Patch none

Description Eric Carlson 2013-09-25 22:05:17 PDT
partial interface HTMLMediaElement
 {
                attribute MediaStream? srcObject;
};




<rdar://problem/15022712>
Comment 1 Nick Diego Yamane (diegoyam) 2013-11-26 11:06:18 PST
Created attachment 217891 [details]
Patch
Comment 2 Thiago de Barros Lacerda 2013-11-26 11:30:51 PST
Comment on attachment 217891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217891&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:324
> +#if ENABLE(MEDIA_STREAM)

Shouldn't this be guarded with ENABLE(VIDEO) too?

> Source/WebCore/html/HTMLMediaElement.cpp:753
> +#if ENABLE(MEDIA_STREAM)

Ditto.

> Source/WebCore/html/HTMLMediaElement.h:147
> +#if ENABLE(MEDIA_STREAM)

Ditto.

> Source/WebCore/html/HTMLMediaElement.h:808
> +#if ENABLE(MEDIA_STREAM)

Ditto.
Comment 3 Eric Carlson 2013-11-26 11:39:30 PST
Comment on attachment 217891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217891&action=review

>> Source/WebCore/html/HTMLMediaElement.cpp:324
>> +#if ENABLE(MEDIA_STREAM)
> 
> Shouldn't this be guarded with ENABLE(VIDEO) too?

Everything in this file is already guarded with ENABLE(VIDEO)

>> Source/WebCore/html/HTMLMediaElement.h:147
>> +#if ENABLE(MEDIA_STREAM)
> 
> Ditto.

Ditto.
Comment 4 Nick Diego Yamane (diegoyam) 2013-11-26 13:01:39 PST
Created attachment 217896 [details]
Added Xcode stuff
Comment 5 Sam Weinig 2013-11-26 13:05:16 PST
Comment on attachment 217891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217891&action=review

> Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.cpp:38
> +    ASSERT(mediaElement);

If this can never be null, can you change the function to take a HTMLMediaElement&.

> Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.cpp:40
> +    isNull = !mediaStream;

This is probably not your doing, but passing null out of band here seems wasteful, since we have an inband signal already (the value of the pointer).

> Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.cpp:46
> +    ASSERT(mediaElement);

If this can never be null, can you change the function to take a HTMLMediaElement&.

> Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.h:40
> +    static PassRefPtr<MediaStream> srcObject(HTMLMediaElement*, bool& isNull);

This should not return a PassRefPtr. Just good old MediaStream*.

> Source/WebCore/html/HTMLMediaElement.h:148
> +    PassRefPtr<MediaStream> srcObject() const { return m_mediaStreamSrcObject; }

This should not return a PassRefPtr. Just good old MediaStream*.
Comment 6 kov's GTK+ EWS bot 2013-11-26 13:48:19 PST
Comment on attachment 217891 [details]
Patch

Attachment 217891 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/37468030
Comment 7 Nick Diego Yamane (diegoyam) 2013-11-26 14:34:41 PST
(In reply to comment #5)
> (From update of attachment 217891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217891&action=review
> 
> > Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.cpp:38
> > +    ASSERT(mediaElement);
> 
> If this can never be null, can you change the function to take a HTMLMediaElement&.

I suspect that's not possible, because this function is called by the auto-generated binding code, which in this case expects to pass a HTMLMediaElement* as the first parameter.

> 
> > Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.cpp:40
> > +    isNull = !mediaStream;
> 
> This is probably not your doing, but passing null out of band here seems wasteful, since we have an inband signal already (the value of the pointer).
> 

'isNull' parameter is also parte of the interface expected by auto-generated code. I'm not sure if it's necessary to set it according to the pointer value...

> > Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.cpp:46
> > +    ASSERT(mediaElement);
> 
> If this can never be null, can you change the function to take a HTMLMediaElement&.
> 
> > Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.h:40
> > +    static PassRefPtr<MediaStream> srcObject(HTMLMediaElement*, bool& isNull);
> 
> This should not return a PassRefPtr. Just good old MediaStream*.
> 
Ok, done.

> > Source/WebCore/html/HTMLMediaElement.h:148
> > +    PassRefPtr<MediaStream> srcObject() const { return m_mediaStreamSrcObject; }
> 
> This should not return a PassRefPtr. Just good old MediaStream*.

ok
Comment 8 Nick Diego Yamane (diegoyam) 2013-11-26 17:32:13 PST
Created attachment 217914 [details]
Proposed Patch

Fixing GTK build issues
Comment 9 WebKit Commit Bot 2013-11-26 18:10:28 PST
Comment on attachment 217914 [details]
Proposed Patch

Clearing flags on attachment: 217914

Committed r159797: <http://trac.webkit.org/changeset/159797>
Comment 10 WebKit Commit Bot 2013-11-26 18:10:33 PST
All reviewed patches have been landed.  Closing bug.