Bug 150449 – [MediaStream] Play MediaStream through media element and render to canvas

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Enable a MediaStream to be played through a media element and rendered to a canvas.

[Eric Carlson]

Created attachment 264192 [details]
Patch for the bots.

[Build Bot]

Comment on attachment 264192 [details]
Patch for the bots.

Attachment 264192 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/347124

New failing tests:
media/video-source-error-no-candidate.html
media/W3C/video/error/error_onerror_called_on_bogus_source.html
http/tests/media/text-served-as-text.html
media/video-src-change.html
media/W3C/audio/error/error_onerror_called_on_bogus_source.html
media/invalid-media-url-crash.html
http/tests/media/pdf-served-as-pdf.html

[Build Bot]

Created attachment 264194 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Eric Carlson]

Created attachment 264214 [details]
Updated patch.

[Eric Carlson]

Created attachment 264215 [details]
Updated patch.

[Eric Carlson]

Created attachment 264217 [details]
Another updated patch.

[Eric Carlson]

Created attachment 264219 [details]
Another updated patch.

[Eric Carlson]

Created attachment 264223 [details]
Another updated patch.

[Jer Noble]

Comment on attachment 264223 [details]
Another updated patch.

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

r=me, with nits.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:264
> +        // FIXME: This can't be right, the event type is set from m_isActive in scheduleActiveStateChange!

I don't see a ChangeLog entry for this. Does this need a bugzilla bug?

> Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp:83
> +MediaStreamPrivate::~MediaStreamPrivate()
> +{
> +}

Don't you need to remove the observers from the tracks in m_trackSet here?

> Source/WebCore/platform/mediastream/MediaStreamPrivate.h:76
> +    MediaStream* publicStream() const { return m_publicStream; }
> +    void setPublicStream(MediaStream* stream) { m_publicStream = stream; }

This is a _little_ concerning. I realize that we're not pulling in MediaStream.h here, just its forward declaration, but it still seems like it may be a layering violation, or at least, would allow layering violations to propagate.

If the point of this method is to allow clients of MediaStreamPrivate to connect to a MediaStream, perhaps we could have a static method in MediaStream, which, when given a MediaStreamPrivate returns the MediaStream associated with that private?

> Source/WebCore/platform/mediastream/mac/AVAudioCaptureSource.mm:84
> +    // FIXME: finish this implementation

bug #?

> Source/WebCore/platform/mediastream/mac/AVAudioCaptureSource.mm:96
> -    {
> -        LockHolder lock(m_lock);
> -        m_observers.append(observer);
> -    }
> -
> +    LockHolder lock(m_lock);
> +    m_observers.append(observer);

The ChangeLog just says "hold the lock while calling observers", but not why.

[Eric Carlson]

Comment on attachment 264223 [details]
Another updated patch.

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

>> Source/WebCore/Modules/mediastream/MediaStream.cpp:264
>> +        // FIXME: This can't be right, the event type is set from m_isActive in scheduleActiveStateChange!
> 
> I don't see a ChangeLog entry for this. Does this need a bugzilla bug?

Fixed.

>> Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp:83
>> +}
> 
> Don't you need to remove the observers from the tracks in m_trackSet here?

Oops, fixed!

>> Source/WebCore/platform/mediastream/MediaStreamPrivate.h:76
>> +    void setPublicStream(MediaStream* stream) { m_publicStream = stream; }
> 
> This is a _little_ concerning. I realize that we're not pulling in MediaStream.h here, just its forward declaration, but it still seems like it may be a layering violation, or at least, would allow layering violations to propagate.
> 
> If the point of this method is to allow clients of MediaStreamPrivate to connect to a MediaStream, perhaps we could have a static method in MediaStream, which, when given a MediaStreamPrivate returns the MediaStream associated with that private?

As discussed on IRC, fixed.

>> Source/WebCore/platform/mediastream/mac/AVAudioCaptureSource.mm:84
>> +    // FIXME: finish this implementation
> 
> bug #?

Added.

>> Source/WebCore/platform/mediastream/mac/AVAudioCaptureSource.mm:96
>> +    m_observers.append(observer);
> 
> The ChangeLog just says "hold the lock while calling observers", but not why.

Fixed.

[Eric Carlson]

Created attachment 264248 [details]
Patch for landing.

[Eric Carlson]

Created attachment 264268 [details]
Updated path for landing.

[WebKit Commit Bot]

Comment on attachment 264268 [details]
Updated path for landing.

Rejecting attachment 264268 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 264268, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/351339

[Eric Carlson]

Created attachment 264297 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 264297 [details]
Patch for landing.

Clearing flags on attachment: 264297

Committed r191721: <http://trac.webkit.org/changeset/191721>

[Csaba Osztrogonác]

(In reply to comment #15)
> Comment on attachment 264297 [details]
> Patch for landing.
> 
> Clearing flags on attachment: 264297
> 
> Committed r191721: <http://trac.webkit.org/changeset/191721>

It made 12 tests timeout on El Capitan bots. 
Could you possibly fix this regression?

[Eric Carlson]

(In reply to comment #16)
> (In reply to comment #15)
> > Comment on attachment 264297 [details]
> > Patch for landing.
> > 
> > Clearing flags on attachment: 264297
> > 
> > Committed r191721: <http://trac.webkit.org/changeset/191721>
> 
> It made 12 tests timeout on El Capitan bots. 
> Could you possibly fix this regression?

I am looking at this.

[Eric Carlson]

It looks like it is a not a problem with this patch per-se, but this patch has uncovered a very old bug in MediaPlayer::getSupportedTypes. Working on a fix.

[Eric Carlson]

I will fix this in bug 150669.