|
|
Created:
3 years, 10 months ago by emircan Modified:
3 years, 10 months ago CC:
chromium-reviews, dshwang, ajuma+watch-canvas_chromium.org, blink-reviews-html_chromium.org, jam, feature-media-reviews_chromium.org, dglazkov+blink, Rik, darin-cc_chromium.org, blink-reviews, emircan+watch+capturefromdom_chromium.org, mcasas+watch+capturefromdom_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNotify listeners on bitmaprenderer context changes
This CL addresses the bug in HTMLCanvasElement::getSourceImageForCanvas() caused
by the uninitialized value of |status|. With the corrected status, we can send
captured frame to canvas listeners.
BUG=679610
TEST=Added layout test checking for a single frame capture and content_browsertest
for a continuous capture.
Review-Url: https://codereview.chromium.org/2683343002
Cr-Commit-Position: refs/heads/master@{#452743}
Committed: https://chromium.googlesource.com/chromium/src/+/fab6dba98d3356e685ffee7ca2cd6acadb2fb4f9
Patch Set 1 : #
Total comments: 9
Patch Set 2 : #Patch Set 3 : Rebase #Patch Set 4 : Change didDraw() call. #
Messages
Total messages: 40 (27 generated)
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== imagebitmaprenderingcontext. BUG= ========== to ========== Notify listeners on bitmaprenderer context changes This CL addresses the bug in HTMLCanvasElement::getSourceImageForCanvas() caused by the uninitialized value of |status|. With the corrected status, we can send captured frame to canvas listeners. BUG=679610 TEST=Added layout test checking for a single frame capture and content_browsertest for a continuous capture. ==========
emircan@chromium.org changed reviewers: + junov@chromium.org
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm for webkit
emircan@chromium.org changed reviewers: + mcasas@chromium.org
Thanks. mcasas@ can you PTAL at /content/*?
tests and content/... lgtm with suggestions https://codereview.chromium.org/2683343002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html (right): https://codereview.chromium.org/2683343002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html:25: var offscreen = new OffscreenCanvas(400, 200); nit: maybe make it smaller? (==faster test?) But not too small because the encoder inside MR might not like it. I'd say 64x64 would be a good number. https://codereview.chromium.org/2683343002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html:29: assert_true(event.data.size > 0, 'Recorded data size should be > 0'); You can safely remove this check here since you shouldn't be concerned about it. https://codereview.chromium.org/2683343002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html:38: console.log('DONE'); Remove stale logging. https://codereview.chromium.org/2683343002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html:40: message; Wrong indent? https://codereview.chromium.org/2683343002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html:48: 'capture of calling transferFromImageBitmap on OffscreenCanvas with WebGL context'); nit: reflow this line and l.45.
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2683343002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html (right): https://codereview.chromium.org/2683343002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html:25: var offscreen = new OffscreenCanvas(400, 200); On 2017/02/15 01:20:27, mcasas wrote: > nit: maybe make it smaller? (==faster test?) > But not too small because the encoder inside MR > might not like it. I'd say 64x64 would be a good > number. Done. I made it 64x48 such that width and height checks are exclusive. https://codereview.chromium.org/2683343002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html:38: console.log('DONE'); On 2017/02/15 01:20:27, mcasas wrote: > Remove stale logging. Done. https://codereview.chromium.org/2683343002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html:40: message; On 2017/02/15 01:20:28, mcasas wrote: > Wrong indent? Done. https://codereview.chromium.org/2683343002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html:48: 'capture of calling transferFromImageBitmap on OffscreenCanvas with WebGL context'); On 2017/02/15 01:20:27, mcasas wrote: > nit: reflow this line and l.45. Done.
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
junov@ these tests no longer pass after https://codereview.chromium.org/2653933003/. I can see that there are didDraw() calls but no finalizeFrame() calls. Do you know why?
On 2017/02/15 21:51:04, emircan wrote: > junov@ these tests no longer pass after > https://codereview.chromium.org/2653933003/. I can see that there are didDraw() > calls but no finalizeFrame() calls. Do you know why? Hmmm... the stack traces are not very useful. I just launched addition try jobs: msan, tsan and dbg linux bot. Maybe we'll get lucky with one of those. Otherwise, best bet to repro the failure locally in a debug build, with a debugger attached.
On 2017/02/15 22:34:29, Justin Novosad wrote: > On 2017/02/15 21:51:04, emircan wrote: > > junov@ these tests no longer pass after > > https://codereview.chromium.org/2653933003/. I can see that there are > didDraw() > > calls but no finalizeFrame() calls. Do you know why? > > Hmmm... the stack traces are not very useful. I just launched addition try > jobs: msan, tsan and dbg linux bot. Maybe we'll get lucky with one of those. > Otherwise, best bet to repro the failure locally in a debug build, with a > debugger attached. It isn't hitting any DCHECKs or such, it just doesn't capture anything and test fails. I can repro it locally. How is HTMLCanvasElement::finalizeFrame() supposed to be called in this case? I can add logs to narrow it down.
I think the missing link is that ImageBitmapRenderingContext::transferFromImageBitmap needs to call CanvasRanderingContext::didDraw() instead of HTMLCanvasElement::didDraw() Just delete the "canvas()->" in front of didDraw.
The CQ bit was checked by emircan@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 emircan@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by emircan@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/21 19:16:35, Justin Novosad wrote: > I think the missing link is that > ImageBitmapRenderingContext::transferFromImageBitmap needs to call > CanvasRanderingContext::didDraw() instead of HTMLCanvasElement::didDraw() > > Just delete the "canvas()->" in front of didDraw. Just did and it passes the test. Can you take a quick look at third_party/WebKit/Source/modules/imagebitmap/ImageBitmapRenderingContext.cpp to verify the change in rect is correct?
Awesome! lgtm.
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2683343002/#ps100001 (title: "Change didDraw() call.")
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": 100001, "attempt_start_ts": 1487898040757750, "parent_rev": "905c681bf73399ab9c462ccf57e9f05640a92277", "commit_rev": "fab6dba98d3356e685ffee7ca2cd6acadb2fb4f9"}
Message was sent while issue was closed.
Description was changed from ========== Notify listeners on bitmaprenderer context changes This CL addresses the bug in HTMLCanvasElement::getSourceImageForCanvas() caused by the uninitialized value of |status|. With the corrected status, we can send captured frame to canvas listeners. BUG=679610 TEST=Added layout test checking for a single frame capture and content_browsertest for a continuous capture. ========== to ========== Notify listeners on bitmaprenderer context changes This CL addresses the bug in HTMLCanvasElement::getSourceImageForCanvas() caused by the uninitialized value of |status|. With the corrected status, we can send captured frame to canvas listeners. BUG=679610 TEST=Added layout test checking for a single frame capture and content_browsertest for a continuous capture. Review-Url: https://codereview.chromium.org/2683343002 Cr-Commit-Position: refs/heads/master@{#452743} Committed: https://chromium.googlesource.com/chromium/src/+/fab6dba98d3356e685ffee7ca2cd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/fab6dba98d3356e685ffee7ca2cd... |