Bug 169192

Summary: Support canvas captureStream
Product: WebKit Reporter: youenn fablet <youennf>
Component: CanvasAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, buildbot, commit-queue, dino, eric.carlson, jer.noble, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Trying to fix build
none
Trying to fix build
none
Fix build
none
Fix build
none
Trying to fix win build
none
Patch for landing none

Description youenn fablet 2017-03-05 22:01:54 PST
Supporting captureStream in the context of webrtc would be useful (testing and as a feature)
Comment 1 youenn fablet 2017-03-05 22:16:46 PST
Created attachment 303498 [details]
Patch
Comment 2 Build Bot 2017-03-05 23:27:35 PST
Comment on attachment 303498 [details]
Patch

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

New failing tests:
media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html
Comment 3 Build Bot 2017-03-05 23:27:38 PST
Created attachment 303499 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 4 youenn fablet 2017-03-06 18:39:04 PST
Created attachment 303599 [details]
Trying to fix build
Comment 5 youenn fablet 2017-03-06 20:30:13 PST
Created attachment 303611 [details]
Trying to fix build
Comment 6 youenn fablet 2017-03-06 20:54:00 PST
Created attachment 303613 [details]
Fix build
Comment 7 youenn fablet 2017-03-06 21:30:50 PST
Created attachment 303618 [details]
Fix build
Comment 8 youenn fablet 2017-03-07 10:35:20 PST
Created attachment 303674 [details]
Trying to fix win build
Comment 9 Alex Christensen 2017-03-07 10:44:10 PST
Comment on attachment 303674 [details]
Trying to fix win build

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

> Source/WebCore/platform/cocoa/CoreVideoSoftLink.h:69
> +using ReleaseCallback = void (*)(void*, const void*);

It seems excessive to give this a name if it's only used once.
Comment 10 Dean Jackson 2017-03-07 13:34:56 PST
Comment on attachment 303674 [details]
Trying to fix win build

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

I don't think the tests will work in their current state.

> Source/WebCore/ChangeLog:17
> +        Tests: fast/mediacapturefromelement/CanvasCaptureMediaStream-2d-events.html
> +               fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html
> +               fast/mediacapturefromelement/CanvasCaptureMediaStream-clone-track.html
> +               fast/mediacapturefromelement/CanvasCaptureMediaStream-creation.html
> +               fast/mediacapturefromelement/CanvasCaptureMediaStream-exceptions.html
> +               fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html
> +               fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html
> +               fast/mediacapturefromelement/CanvasCaptureMediaStream-offscreencanvas.html
> +               fast/mediacapturefromelement/CanvasCaptureMediaStream-request-frame-events.html
> +               fast/mediacapturefromelement/CanvasCaptureMediaStream-webgl-events.html

I wonder if this should be fast/mediacapture/fromelement

> Source/WebCore/ChangeLog:48
> +        * bindings/js/JSMediaStreamTrackCustom.cpp: Copied from Source/WebCore/html/HTMLCanvasElement.idl.

This generated line seems wrong.

> Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:85
> +    if (m_frameRequestRate.value())
> +        m_requestFrameTimer.startRepeating(1. / m_frameRequestRate.value());

I wonder if we can do better here, by hooking into the rAF machinery if the frame rate is 60. Otherwise we'll be slightly out of sync.

> Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:98
> +    if (!m_canvas)
> +        return;
> +    m_canvas->removeObserver(*this);
> +
> +    m_requestFrameTimer.stop();

While it doesn't matter here, it might be better to have these the other way around. i.e. reset in the reverse order

> Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:111
> +    m_requestFrameTimer.stop();
> +    stopProducingData();

No need to cancel the timer here since stopProducingData() does it.

> Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:127
> +    captureCanvas();

Wouldn't this trigger a capture even in the manual case? Does the spec say this should happen?

I would have thought you'd just wait until captureCanvas is called by the timer or manually.

> Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:171
> +    context.setImageInterpolationQuality(InterpolationLow);

Why low?

> Source/WebCore/platform/graphics/ImageBuffer.h:116
> +    Vector<uint8_t> toBGRAData() const;

Could we use JSC::Uint8Array instead? Does it matter?

> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:110
> +    // FIXME:Add support for not ACCELERATE.

You'll need UNUSED_PARAMS here too.

> LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-2d-events-expected.txt:10
> +PASS Plugged stream to video tag.
> +

Shouldn't we also see the testPassed('Video canplaythrough callback succeeded.') or testPassed('Video play callback succeeded.') calls?

> LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-2d-events.html:54
> +  video.play();

I assume this works on iOS because there is no audio track?

> LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html:19
> +// Run captureStream() on OffscreenCanvas that uses transferToImageBitmap().

We don't implement these yet.
Comment 11 youenn fablet 2017-03-08 09:29:24 PST
Thanks for the reviews.

(In reply to comment #9)
> Comment on attachment 303674 [details]
> Trying to fix win build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303674&action=review
> 
> > Source/WebCore/platform/cocoa/CoreVideoSoftLink.h:69
> > +using ReleaseCallback = void (*)(void*, const void*);
> 
> It seems excessive to give this a name if it's only used once.

Done

> I don't think the tests will work in their current state.

Some of them do not.
I did not change them since they are copied directly from chromium.
We might end up implementing MediaRecoder.
Maybe these tests should be moved to "imported"

> > Source/WebCore/ChangeLog:17
> > +        Tests: fast/mediacapturefromelement/CanvasCaptureMediaStream-2d-events.html
> > +               fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html
> > +               fast/mediacapturefromelement/CanvasCaptureMediaStream-clone-track.html
> > +               fast/mediacapturefromelement/CanvasCaptureMediaStream-creation.html
> > +               fast/mediacapturefromelement/CanvasCaptureMediaStream-exceptions.html
> > +               fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html
> > +               fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html
> > +               fast/mediacapturefromelement/CanvasCaptureMediaStream-offscreencanvas.html
> > +               fast/mediacapturefromelement/CanvasCaptureMediaStream-request-frame-events.html
> > +               fast/mediacapturefromelement/CanvasCaptureMediaStream-webgl-events.html
> 
> I wonder if this should be fast/mediacapture/fromelement

I kept the same hierarchy as chromium for easier future reimporting.


> 
> > Source/WebCore/ChangeLog:48
> > +        * bindings/js/JSMediaStreamTrackCustom.cpp: Copied from Source/WebCore/html/HTMLCanvasElement.idl.
> 
> This generated line seems wrong.

OK

> > Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:85
> > +    if (m_frameRequestRate.value())
> > +        m_requestFrameTimer.startRepeating(1. / m_frameRequestRate.value());
> 
> I wonder if we can do better here, by hooking into the rAF machinery if the
> frame rate is 60. Otherwise we'll be slightly out of sync.

m_frameRequestRate is used for throttling.
Setting it to 60 just means that the max frame rate will be 60.
If no drawing happens, no frame will be rendered

> > Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:98
> > +    if (!m_canvas)
> > +        return;
> > +    m_canvas->removeObserver(*this);
> > +
> > +    m_requestFrameTimer.stop();
> 
> While it doesn't matter here, it might be better to have these the other way
> around. i.e. reset in the reverse order

OK

> 
> > Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:111
> > +    m_requestFrameTimer.stop();
> > +    stopProducingData();
> 
> No need to cancel the timer here since stopProducingData() does it.

OK

> > Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:127
> > +    captureCanvas();
> 
> Wouldn't this trigger a capture even in the manual case? Does the spec say
> this should happen?

We will only capture if m_shouldEmitFrame is true.
In the manual case, it will e set to true after requestFrame() be called.

> I would have thought you'd just wait until captureCanvas is called by the
> timer or manually.
> 
> > Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:171
> > +    context.setImageInterpolationQuality(InterpolationLow);
> 
> Why low?

This is what is used for media players.
Not sure what the best value is here.

> 
> > Source/WebCore/platform/graphics/ImageBuffer.h:116
> > +    Vector<uint8_t> toBGRAData() const;
> 
> Could we use JSC::Uint8Array instead? Does it matter?

The data will never surface as JS objects so it seems unnecessary to use a JS array here.

Ideally captureStream should be made a streams API object, like a ReadableByteStream.
And we should pipe it to create a MediaStreamTrack.
That way, we could expose the raw data to user scripts and be able to optimize it if used as a MediaStreamTrack.

> 
> > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:110
> > +    // FIXME:Add support for not ACCELERATE.
> 
> You'll need UNUSED_PARAMS here too.

OK

> 
> > LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-2d-events-expected.txt:10
> > +PASS Plugged stream to video tag.
> > +
> 
> Shouldn't we also see the testPassed('Video canplaythrough callback
> succeeded.') or testPassed('Video play callback succeeded.') calls?

Right.
When test is run in browser, it shows this.
In rwt, it does not show up. Might be due to differences like autoplay setup between the two?

> 
> > LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-2d-events.html:54
> > +  video.play();
> 
> I assume this works on iOS because there is no audio track?
> 
> > LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html:19
> > +// Run captureStream() on OffscreenCanvas that uses transferToImageBitmap().
> 
> We don't implement these yet.

Right.
Comment 12 youenn fablet 2017-03-08 14:03:58 PST
Created attachment 303838 [details]
Patch for landing
Comment 13 youenn fablet 2017-03-08 14:04:45 PST
(In reply to comment #12)
> Created attachment 303838 [details]
> Patch for landing

Adding a new test and fixing a bug in the copy of the data (bad bytesPerRow).
Comment 14 WebKit Commit Bot 2017-03-08 14:44:46 PST
Comment on attachment 303838 [details]
Patch for landing

Clearing flags on attachment: 303838

Committed r213598: <http://trac.webkit.org/changeset/213598>
Comment 15 WebKit Commit Bot 2017-03-08 14:44:52 PST
All reviewed patches have been landed.  Closing bug.