Bug 169192 – Support canvas captureStream

• Component: Canvas
• Status: RESOLVED FIXED

[youenn fablet]

Supporting captureStream in the context of webrtc would be useful (testing and as a feature)

[youenn fablet]

Created attachment 303498 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

Created attachment 303599 [details]
Trying to fix build

[youenn fablet]

Created attachment 303611 [details]
Trying to fix build

[youenn fablet]

Created attachment 303613 [details]
Fix build

[youenn fablet]

Created attachment 303618 [details]
Fix build

[youenn fablet]

Created attachment 303674 [details]
Trying to fix win build

[Alex Christensen]

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.

[Dean Jackson]

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.

[youenn fablet]

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.

[youenn fablet]

Created attachment 303838 [details]
Patch for landing

[youenn fablet]

(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).

[WebKit Commit Bot]

Comment on attachment 303838 [details]
Patch for landing

Clearing flags on attachment: 303838

Committed r213598: <http://trac.webkit.org/changeset/213598>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.