Bug 170325 - captureStream is getting black frames with webgl canvas
Summary: captureStream is getting black frames with webgl canvas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 211156
  Show dependency treegraph
 
Reported: 2017-03-30 21:45 PDT by youenn fablet
Modified: 2020-07-22 11:25 PDT (History)
4 users (show)

See Also:


Attachments
Patch (28.82 KB, patch)
2017-03-30 22:02 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing release builds (28.83 KB, patch)
2017-03-31 09:58 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Updated according review (29.25 KB, patch)
2017-03-31 16:08 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (30.14 KB, patch)
2017-04-03 09:18 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-03-30 21:45:40 PDT
captureStream is getting black frames with webgl canvas
Comment 1 youenn fablet 2017-03-30 22:02:07 PDT
Created attachment 305941 [details]
Patch
Comment 2 youenn fablet 2017-03-30 22:03:06 PDT
(In reply to youenn fablet from comment #0)
> captureStream is getting black frames with webgl canvas

Regression is due to the timer introduced to not produce too many frames.
Comment 3 youenn fablet 2017-03-31 09:58:06 PDT
Created attachment 305979 [details]
Fixing release builds
Comment 4 Dean Jackson 2017-03-31 15:08:14 PDT
Comment on attachment 305979 [details]
Fixing release builds

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

> Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:136
> +    if (canvas.renderingContext() && canvas.renderingContext()->isWebGL())
> +        static_cast<WebGLRenderingContextBase*>(canvas.renderingContext())->preserveDrawingBuffer();

Doing this does have a negative performance effect on the WebGL context. Maybe until we have the recording synced with the endPaint, we should print a warning to the console saying this might be slow (if the context wasn't already created with preserveDrawingBuffer = true)

> Source/WebCore/html/canvas/WebGLRenderingContextBase.h:210
> +    void preserveDrawingBuffer() { m_attributes.preserveDrawingBuffer = true; }

I think this should be setPreserveDrawingBuffer(bool), with a preserveDrawingBuffer() getter.

> Source/WebCore/testing/Internals.h:590
> +    std::optional<TrackFramePromise> m_nextFramePromise;

Maybe m_nextTrackFramePromise?
Comment 5 youenn fablet 2017-03-31 16:08:56 PDT
Created attachment 306023 [details]
Updated according review
Comment 6 youenn fablet 2017-04-03 08:57:25 PDT
(In reply to Dean Jackson from comment #4)
> Comment on attachment 305979 [details]
> Fixing release builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305979&action=review
> 
> > Source/WebCore/Modules/mediastream/CanvasCaptureMediaStreamTrack.cpp:136
> > +    if (canvas.renderingContext() && canvas.renderingContext()->isWebGL())
> > +        static_cast<WebGLRenderingContextBase*>(canvas.renderingContext())->preserveDrawingBuffer();
> 
> Doing this does have a negative performance effect on the WebGL context.
> Maybe until we have the recording synced with the endPaint, we should print
> a warning to the console saying this might be slow (if the context wasn't
> already created with preserveDrawingBuffer = true)

Added

> 
> > Source/WebCore/html/canvas/WebGLRenderingContextBase.h:210
> > +    void preserveDrawingBuffer() { m_attributes.preserveDrawingBuffer = true; }
> 
> I think this should be setPreserveDrawingBuffer(bool), with a
> preserveDrawingBuffer() getter.

OK

> > Source/WebCore/testing/Internals.h:590
> > +    std::optional<TrackFramePromise> m_nextFramePromise;
> 
> Maybe m_nextTrackFramePromise?

OK
Comment 7 youenn fablet 2017-04-03 09:18:41 PDT
Created attachment 306076 [details]
Patch for landing
Comment 8 youenn fablet 2017-04-03 09:19:45 PDT
Thanks for the review.

> Doing this does have a negative performance effect on the WebGL context.
> Maybe until we have the recording synced with the endPaint, we should print
> a warning to the console saying this might be slow (if the context wasn't
> already created with preserveDrawingBuffer = true)

I added the log message and added a test to cover "preserveDrawingBuffer = true" case.
Comment 9 WebKit Commit Bot 2017-04-03 09:59:44 PDT
Comment on attachment 306076 [details]
Patch for landing

Clearing flags on attachment: 306076

Committed r214806: <http://trac.webkit.org/changeset/214806>
Comment 10 WebKit Commit Bot 2017-04-03 09:59:46 PDT
All reviewed patches have been landed.  Closing bug.