Enable a MediaStream to be played through a media element and rendered to a canvas.
Created attachment 264192 [details] Patch for the bots.
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
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
Created attachment 264214 [details] Updated patch.
Created attachment 264215 [details] Updated patch.
Created attachment 264217 [details] Another updated patch.
Created attachment 264219 [details] Another updated patch.
Created attachment 264223 [details] Another updated patch.
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.
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.
Created attachment 264248 [details] Patch for landing.
Created attachment 264268 [details] Updated path for landing.
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
Created attachment 264297 [details] Patch for landing.
Comment on attachment 264297 [details] Patch for landing. Clearing flags on attachment: 264297 Committed r191721: <http://trac.webkit.org/changeset/191721>
(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?
(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.
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.
I will fix this in bug 150669.