Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2768683002:
Fix unreliable MediaStream capture from WebGL canvases (Closed)
|
Created:
3 years, 9 months ago by Justin Novosad Modified:
3 years, 8 months ago CC:
chromium-reviews, dshwang, ajuma+watch-canvas_chromium.org, blink-reviews-html_chromium.org, mcasas+capturefromelement_chromium.org, dglazkov+blink, Rik, blink-reviews, emircan+watch+capturefromdom_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix unreliable MediaStream capture from WebGL canvases
When capturing a MediaStrem from a WebGL canvas that
has the preserveDrawingBuffer option set to false, it is important
to perform the frame capture before the canvas is cleared by the
buffer swap mechanism. This change resolves the issue by performing
the capture during the paint invalidation phase instead of during
frame finalization. For canvases that are not visible on screen,
there is no paint invalidation, which is a problem. So in that
case, we fall back to doing the capture during frame finalization,
which is okay in that case because there is no buffer swap.
BUG=702446
Review-Url: https://codereview.chromium.org/2768683002
Cr-Commit-Position: refs/heads/master@{#460159}
Committed: https://chromium.googlesource.com/chromium/src/+/7b09536cad07c7983eac63eac07ecf82f53c65d1
Patch Set 1 #Patch Set 2 : comment typo fix #Patch Set 3 : fix test flakiness #Patch Set 4 : simplify test by removing WebRTC connection #
Total comments: 2
Patch Set 5 : Replace LayoutTest with browser test #Patch Set 6 : Boost test tolerance to make it pass on MacOS 10.9 #Patch Set 7 : mac suppress #
Messages
Total messages: 45 (27 generated)
The CQ bit was checked by junov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
junov@chromium.org changed reviewers: + emircan@chromium.org, xlai@chromium.org
@emircan: please review the test @xlai: please review the code
Description was changed from ========== Fix unreliable MediaStream capture from WebGL canvases When capturing a MediaStrem from a canvas WebGL canvas that has the preserveDrawingBuffer option set to false, it is important to perform the frame capture before the canvas is cleared by the buffer swap mechanism. This change resolve the issue by performing the capture during the paint invalidation phase instead of during frame finalization. For canvases that are no visible on screen, there is no paint invalidation, which is a problem. So in that case, we fallback to doing the capture during frame finalization, which is okay in that case because there is no buffer swap. BUG=702446 ========== to ========== Fix unreliable MediaStream capture from WebGL canvases When capturing a MediaStrem from a WebGL canvas that has the preserveDrawingBuffer option set to false, it is important to perform the frame capture before the canvas is cleared by the buffer swap mechanism. This change resolve the issue by performing the capture during the paint invalidation phase instead of during frame finalization. For canvases that are no visible on screen, there is no paint invalidation, which is a problem. So in that case, we fallback to doing the capture during frame finalization, which is okay in that case because there is no buffer swap. BUG=702446 ==========
Description was changed from ========== Fix unreliable MediaStream capture from WebGL canvases When capturing a MediaStrem from a WebGL canvas that has the preserveDrawingBuffer option set to false, it is important to perform the frame capture before the canvas is cleared by the buffer swap mechanism. This change resolve the issue by performing the capture during the paint invalidation phase instead of during frame finalization. For canvases that are no visible on screen, there is no paint invalidation, which is a problem. So in that case, we fallback to doing the capture during frame finalization, which is okay in that case because there is no buffer swap. BUG=702446 ========== to ========== Fix unreliable MediaStream capture from WebGL canvases When capturing a MediaStrem from a WebGL canvas that has the preserveDrawingBuffer option set to false, it is important to perform the frame capture before the canvas is cleared by the buffer swap mechanism. This change resolves the issue by performing the capture during the paint invalidation phase instead of during frame finalization. For canvases that are not visible on screen, there is no paint invalidation, which is a problem. So in that case, we fall back to doing the capture during frame finalization, which is okay in that case because there is no buffer swap. BUG=702446 ==========
The CQ bit was checked by junov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by junov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Also is there a scenario when canvas becomes visible and invisible back and forth? If so does your code considers that scenario? https://codereview.chromium.org/2768683002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/2768683002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:342: m_didNotifyListenersForCurrentFrame = false; I'm thinking of a scenario when the canvas is visible and finalizeFrame() and doPaintInvalidation() occur interleaving each other...Then the m_didNotifyListenersForCurrentFrame is set to true and false back and forth...Is that what you intend to do?
On 2017/03/22 17:06:21, xlai (Olivia) wrote: > Also is there a scenario when canvas becomes visible and invisible back and > forth? If so does your code considers that scenario? > > https://codereview.chromium.org/2768683002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): > > https://codereview.chromium.org/2768683002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:342: > m_didNotifyListenersForCurrentFrame = false; > I'm thinking of a scenario when the canvas is visible and finalizeFrame() and > doPaintInvalidation() occur interleaving each other...Then the > m_didNotifyListenersForCurrentFrame is set to true and false back and forth...Is > that what you intend to do? Yes, that is exactly what is intended. finalizeFrame is called on every frame, regardless of visibility. doPaintInvalidation is called only if the canvas is visible. When the canvas is visible, we want doPaintInvalidation to trigger the capture, so that it happens before the canvas is cleared. By the time finalizeFrame is called, we only want to trigger a frame capture, if a capture has not already been triggered (in doPaintInvalidation), which allows the capture to work in cases where the canvas is not visible.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/03/22 17:20:50, Justin Novosad wrote: > On 2017/03/22 17:06:21, xlai (Olivia) wrote: > > Also is there a scenario when canvas becomes visible and invisible back and > > forth? If so does your code considers that scenario? > > I think that answers this question too. > > > https://codereview.chromium.org/2768683002/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right): > > > > > https://codereview.chromium.org/2768683002/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:342: > > m_didNotifyListenersForCurrentFrame = false; > > I'm thinking of a scenario when the canvas is visible and finalizeFrame() and > > doPaintInvalidation() occur interleaving each other...Then the > > m_didNotifyListenersForCurrentFrame is set to true and false back and > forth...Is > > that what you intend to do? > > Yes, that is exactly what is intended. finalizeFrame is called on every frame, > regardless of visibility. doPaintInvalidation is called only if the canvas is > visible. When the canvas is visible, we want doPaintInvalidation to trigger the > capture, so that it happens before the canvas is cleared. By the time > finalizeFrame is called, we only want to trigger a frame capture, if a capture > has not already been triggered (in doPaintInvalidation), which allows the > capture to work in cases where the canvas is not visible. Okay, so I understand this as "within 1 animation frame cycle, the doPaintInvalidation happens before the finalizeFrame is called for that particular frame, and so notify() gets called in paintInvalidation. But you also want finalizeFrame() to set the m_didNotifyListenersForCurrentFrame back to false for correct behavior of next frame cycle." lgtm for that.
https://codereview.chromium.org/2768683002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-webgl-preservedrawingbuffer-false.html (right): https://codereview.chromium.org/2768683002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-webgl-preservedrawingbuffer-false.html:36: testCtx.drawImage(video, 0, 0); Sorry I forgot about this earlier when we talked about the test. Unfortunately, getImageData from <video> won't work here. For layout tests, MediastreamVideoRenderer uses a tests instance where it just generates frames, see https://cs.chromium.org/chromium/src/content/shell/renderer/layout_test/test_.... So the rendered video is only these generated white frames and doesn't match the input. I am not sure how to get around that in layout tests. MediaRecorder gets the actual frames but you cannot check the color there. Therefore, we added the color check tests earlier as content_browsertests, see https://cs.chromium.org/chromium/src/content/test/data/media/canvas_capture_c....
On 2017/03/22 18:26:27, emircan wrote: > https://codereview.chromium.org/2768683002/diff/60001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-webgl-preservedrawingbuffer-false.html > (right): > > https://codereview.chromium.org/2768683002/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-webgl-preservedrawingbuffer-false.html:36: > testCtx.drawImage(video, 0, 0); > Sorry I forgot about this earlier when we talked about the test. Unfortunately, > getImageData from <video> won't work here. For layout tests, > MediastreamVideoRenderer uses a tests instance where it just generates frames, > see > https://cs.chromium.org/chromium/src/content/shell/renderer/layout_test/test_.... > So the rendered video is only these generated white frames and doesn't match the > input. > > I am not sure how to get around that in layout tests. MediaRecorder gets the > actual frames but you cannot check the color there. Therefore, we added the > color check tests earlier as content_browsertests, see > https://cs.chromium.org/chromium/src/content/test/data/media/canvas_capture_c.... Ah, that explains why the test passes on a chromium build, but fails when run as a layout test. I guess I will just move this test to content_browsertests. Thanks!
The CQ bit was checked by junov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@emircan: I changed the test.
On 2017/03/22 20:58:46, Justin Novosad wrote: > @emircan: I changed the test. lgtm. Thank you very much.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by junov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xlai@chromium.org Link to the patchset: https://codereview.chromium.org/2768683002/#ps80001 (title: "Replace LayoutTest with browser test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by junov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org, xlai@chromium.org Link to the patchset: https://codereview.chromium.org/2768683002/#ps100001 (title: "Boost test tolerance to make it pass on MacOS 10.9")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
There is definitely something wrong on mac. More investigation is needed.
On 2017/03/24 17:29:53, Justin Novosad wrote: > There is definitely something wrong on mac. More investigation is needed. I cannot repro the test failure on a local mac build. I also verified manually that the flickering is in fact fixed on mac. I think that for now I will just land this CL with the test suppressed on mac. In order to not lose any test coverage, I will try splitting the test into two parts (2d vs. WebGL) and will only suppress the WebGL part.
The CQ bit was checked by junov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org, xlai@chromium.org Link to the patchset: https://codereview.chromium.org/2768683002/#ps120001 (title: "mac suppress")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490717023693330,
"parent_rev": "adaaccd59252c8fb9af08fcf65e8b78f25f053b2", "commit_rev":
"7b09536cad07c7983eac63eac07ecf82f53c65d1"}
Message was sent while issue was closed.
Description was changed from ========== Fix unreliable MediaStream capture from WebGL canvases When capturing a MediaStrem from a WebGL canvas that has the preserveDrawingBuffer option set to false, it is important to perform the frame capture before the canvas is cleared by the buffer swap mechanism. This change resolves the issue by performing the capture during the paint invalidation phase instead of during frame finalization. For canvases that are not visible on screen, there is no paint invalidation, which is a problem. So in that case, we fall back to doing the capture during frame finalization, which is okay in that case because there is no buffer swap. BUG=702446 ========== to ========== Fix unreliable MediaStream capture from WebGL canvases When capturing a MediaStrem from a WebGL canvas that has the preserveDrawingBuffer option set to false, it is important to perform the frame capture before the canvas is cleared by the buffer swap mechanism. This change resolves the issue by performing the capture during the paint invalidation phase instead of during frame finalization. For canvases that are not visible on screen, there is no paint invalidation, which is a problem. So in that case, we fall back to doing the capture during frame finalization, which is okay in that case because there is no buffer swap. BUG=702446 Review-Url: https://codereview.chromium.org/2768683002 Cr-Commit-Position: refs/heads/master@{#460159} Committed: https://chromium.googlesource.com/chromium/src/+/7b09536cad07c7983eac63eac07e... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/7b09536cad07c7983eac63eac07e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
