Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1692773002:
Canvas Capture: chrome browsertest- added frame rate test (Closed)
|
Created:
4 years, 10 months ago by cpaulin (no longer in chrome) Modified:
4 years, 9 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, tnakamura+watch_chromium.org, phoglund+watch_chromium.org, mcasas+watch_chromium.org, Lei Zhang Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCanvas Capture: chrome browsertest- added frame rate test
This CL introduces a new chrome browser test for canvas capture.
The test verifies that the canvas capture frame rate is as expected.
BUG=524218, 586316
TEST=On linux host and trybots
Committed: https://crrev.com/7a05c134dedd0137c23ba43b42db8f1e513cd9e6
Cr-Commit-Position: refs/heads/master@{#379065}
Patch Set 1 : New frame rate test for canvas capture. #
Total comments: 14
Patch Set 2 : Addressed phoglund@'s comments. #Patch Set 3 : Fixed incorrect logging of tolerance percentage. #
Total comments: 3
Patch Set 4 : Addressed emircan@'s comments and phoglund@'s comment #Patch Set 5 : Excluding mac trybots for now. #
Total comments: 32
Messages
Total messages: 41 (18 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Canvas Capture: chrome browsertest- added frame rate test This CL introduces a new chrome browser test for canvas capture. The test verifies that the canvas capture frame rate is as expected. BUG=586316 TEST=On linux host and trybots ========== to ========== Canvas Capture: chrome browsertest- added frame rate test This CL introduces a new chrome browser test for canvas capture. The test verifies that the canvas capture frame rate is as expected. BUG=586316 TEST=On linux host and trybots ==========
cpaulin@chromium.org changed reviewers: + phoglund@chromium.org
Hi, Canvas Capture: added frame rate test. My first browser test, hopefully not too much off.
I think the code is fine in general, but you need to think very carefully about what you want to test here. If the draws you make to the canvas are important to the detected FPS count (I guess it is), you need to write your drawing code very carefully. https://codereview.chromium.org/1692773002/diff/80001/chrome/browser/media/ch... File chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc (right): https://codereview.chromium.org/1692773002/diff/80001/chrome/browser/media/ch... chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc:31: class WebRtcCanvasCaptureBrowserTest : public WebRtcTestBase { It doesn't make sense to inherit from WebRtcTestBase here. That class gives you access to the small javascript library in chrome/test/data/webrtc (e.g. getUserMedia, peer connections), none of which you use here. Just inherit from InProcessBrowserTest. You'll have to do without DetectErrorsInJavascript. That one was more useful to us in the past when we had much more javascript, much of which executed in callback functions. I don't think it will be super useful to you, and it doesn't work anyway if the test isn't MANUAL-tagged and running on bots with sequential execution. https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... File chrome/test/data/media/canvas_capture_test.html (right): https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... chrome/test/data/media/canvas_capture_test.html:8: <video id='local_video' muted='true' hidden loop autoplay> In javascript, IDs should be on the form local-video AFAIK https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... chrome/test/data/media/canvas_capture_test.html:22: var ERROR_RATE = 15 / 100; TOLERANCE_PERCENT would be a better name for this one. https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... chrome/test/data/media/canvas_capture_test.html:25: document.addEventListener('DOMContentLoaded', function() { It's more common to use document.onload for this (I suspect it's the same as what you're doing) https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... chrome/test/data/media/canvas_capture_test.html:38: setTimeout(draw, 20, video, context, width, height); Is it important for the fps computations that the next draw executes right on time? setTimeout is pretty unreliable, and even more so on busy bots, which execute tests in parallel. The 20 ms just means it will take _at least_ 20 ms before the next draw executes. It could just as well take 100 ms or even more. http://www.sitepoint.com/creating-accurate-timers-in-javascript/ has more context on this. You may want to look into requestAnimationFrame as well. https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... chrome/test/data/media/canvas_capture_test.html:69: // This functi0on calculates the FPS for decoded frames and dropped frames. Nit: function https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... chrome/test/data/media/canvas_capture_test.html:105: reportResult('FAIL', 'ERROR: FPS value error beyond acceptable value'); Simplify this. It's better to just return the second string to the test. When it fails, it will not just say "FAIL" but "ERROR: FPS value error beyond acceptable value". This gives better context to the test failure. Returning anything else than OK to the test will cause a failure. You should also give even better context, like "Expected FPS between 8 and 12 (15% tolerance), but was 3", or something like that.
On 2016/02/12 08:38:53, phoglund_chrome wrote: > I think the code is fine in general, but you need to think very carefully about > what you want to test here. If the draws you make to the canvas are important to > the detected FPS count (I guess it is), you need to write your drawing code very > carefully. > > https://codereview.chromium.org/1692773002/diff/80001/chrome/browser/media/ch... > File chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc (right): > > https://codereview.chromium.org/1692773002/diff/80001/chrome/browser/media/ch... > chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc:31: class > WebRtcCanvasCaptureBrowserTest : public WebRtcTestBase { > It doesn't make sense to inherit from WebRtcTestBase here. That class gives you > access to the small javascript library in chrome/test/data/webrtc (e.g. > getUserMedia, peer connections), none of which you use here. Just inherit from > InProcessBrowserTest. > > You'll have to do without DetectErrorsInJavascript. That one was more useful to > us in the past when we had much more javascript, much of which executed in > callback functions. I don't think it will be super useful to you, and it doesn't > work anyway if the test isn't MANUAL-tagged and running on bots with sequential > execution. > > https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... > File chrome/test/data/media/canvas_capture_test.html (right): > > https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... > chrome/test/data/media/canvas_capture_test.html:8: <video id='local_video' > muted='true' hidden loop autoplay> > In javascript, IDs should be on the form local-video AFAIK > > https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... > chrome/test/data/media/canvas_capture_test.html:22: var ERROR_RATE = 15 / 100; > TOLERANCE_PERCENT would be a better name for this one. > > https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... > chrome/test/data/media/canvas_capture_test.html:25: > document.addEventListener('DOMContentLoaded', function() { > It's more common to use document.onload for this (I suspect it's the same as > what you're doing) > > https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... > chrome/test/data/media/canvas_capture_test.html:38: setTimeout(draw, 20, video, > context, width, height); > Is it important for the fps computations that the next draw executes right on > time? setTimeout is pretty unreliable, and even more so on busy bots, which > execute tests in parallel. The 20 ms just means it will take _at least_ 20 ms > before the next draw executes. It could just as well take 100 ms or even more. > > http://www.sitepoint.com/creating-accurate-timers-in-javascript/ has more > context on this. You may want to look into requestAnimationFrame as well. > > https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... > chrome/test/data/media/canvas_capture_test.html:69: // This functi0on calculates > the FPS for decoded frames and dropped frames. > Nit: function > > https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... > chrome/test/data/media/canvas_capture_test.html:105: reportResult('FAIL', > 'ERROR: FPS value error beyond acceptable value'); > Simplify this. It's better to just return the second string to the test. When it > fails, it will not just say "FAIL" but "ERROR: FPS value error beyond acceptable > value". This gives better context to the test failure. Returning anything else > than OK to the test will cause a failure. > > You should also give even better context, like "Expected FPS between 8 and 12 > (15% tolerance), but was 3", or something like that. Thanks for the comments, I will look into RAF
Patch set 2 is ready for review. https://codereview.chromium.org/1692773002/diff/80001/chrome/browser/media/ch... File chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc (right): https://codereview.chromium.org/1692773002/diff/80001/chrome/browser/media/ch... chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc:31: class WebRtcCanvasCaptureBrowserTest : public WebRtcTestBase { On 2016/02/12 08:38:53, phoglund_chrome wrote: > It doesn't make sense to inherit from WebRtcTestBase here. That class gives you > access to the small javascript library in chrome/test/data/webrtc (e.g. > getUserMedia, peer connections), none of which you use here. Just inherit from > InProcessBrowserTest. > > You'll have to do without DetectErrorsInJavascript. That one was more useful to > us in the past when we had much more javascript, much of which executed in > callback functions. I don't think it will be super useful to you, and it doesn't > work anyway if the test isn't MANUAL-tagged and running on bots with sequential > execution. Done. https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... File chrome/test/data/media/canvas_capture_test.html (right): https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... chrome/test/data/media/canvas_capture_test.html:8: <video id='local_video' muted='true' hidden loop autoplay> On 2016/02/12 08:38:53, phoglund_chrome wrote: > In javascript, IDs should be on the form local-video AFAIK Done. https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... chrome/test/data/media/canvas_capture_test.html:22: var ERROR_RATE = 15 / 100; On 2016/02/12 08:38:53, phoglund_chrome wrote: > TOLERANCE_PERCENT would be a better name for this one. Done. https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... chrome/test/data/media/canvas_capture_test.html:25: document.addEventListener('DOMContentLoaded', function() { On 2016/02/12 08:38:53, phoglund_chrome wrote: > It's more common to use document.onload for this (I suspect it's the same as > what you're doing) I am using window.onload now Done. https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... chrome/test/data/media/canvas_capture_test.html:38: setTimeout(draw, 20, video, context, width, height); On 2016/02/12 08:38:53, phoglund_chrome wrote: > Is it important for the fps computations that the next draw executes right on > time? setTimeout is pretty unreliable, and even more so on busy bots, which > execute tests in parallel. The 20 ms just means it will take _at least_ 20 ms > before the next draw executes. It could just as well take 100 ms or even more. > > http://www.sitepoint.com/creating-accurate-timers-in-javascript/ has more > context on this. You may want to look into requestAnimationFrame as well. Thanks for the tip, I am using requestAnimationFrame now. Done. https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... chrome/test/data/media/canvas_capture_test.html:69: // This functi0on calculates the FPS for decoded frames and dropped frames. On 2016/02/12 08:38:53, phoglund_chrome wrote: > Nit: function Done. https://codereview.chromium.org/1692773002/diff/80001/chrome/test/data/media/... chrome/test/data/media/canvas_capture_test.html:105: reportResult('FAIL', 'ERROR: FPS value error beyond acceptable value'); On 2016/02/12 08:38:53, phoglund_chrome wrote: > Simplify this. It's better to just return the second string to the test. When it > fails, it will not just say "FAIL" but "ERROR: FPS value error beyond acceptable > value". This gives better context to the test failure. Returning anything else > than OK to the test will cause a failure. > > You should also give even better context, like "Expected FPS between 8 and 12 > (15% tolerance), but was 3", or something like that. That's true, I fixed it. Done.
lgtm https://codereview.chromium.org/1692773002/diff/120001/chrome/test/data/media... File chrome/test/data/media/canvas_capture_test.html (right): https://codereview.chromium.org/1692773002/diff/120001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:113: averageDroppedFPS + ' instead'); Nit: indent 4
cpaulin@chromium.org changed reviewers: + emircan@chromium.org
Emircan, Please review canvas capture browser test Thanks Christian
lgtm % a question. https://codereview.chromium.org/1692773002/diff/120001/chrome/test/data/media... File chrome/test/data/media/canvas_capture_test.html (right): https://codereview.chromium.org/1692773002/diff/120001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:113: averageDroppedFPS + ' instead'); If there are many dropped frames by video, that doesn't necessarily mean smt is wrong with canvas capture. I understand that it is good to report it. However, for the sake of testing canvas capture frame rate, can we add this count or re-run?
https://codereview.chromium.org/1692773002/diff/120001/chrome/test/data/media... File chrome/test/data/media/canvas_capture_test.html (right): https://codereview.chromium.org/1692773002/diff/120001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:113: averageDroppedFPS + ' instead'); On 2016/02/16 22:21:47, emircan wrote: > If there are many dropped frames by video, that doesn't necessarily mean smt is > wrong with canvas capture. I understand that it is good to report it. However, > for the sake of testing canvas capture frame rate, can we add this count or > re-run? Emircan, is it possible that dropped frames are due to canvas capture?
Description was changed from ========== Canvas Capture: chrome browsertest- added frame rate test This CL introduces a new chrome browser test for canvas capture. The test verifies that the canvas capture frame rate is as expected. BUG=586316 TEST=On linux host and trybots ========== to ========== Canvas Capture: chrome browsertest- added frame rate test This CL introduces a new chrome browser test for canvas capture. The test verifies that the canvas capture frame rate is as expected. BUG=524218, 586316 TEST=On linux host and trybots ==========
I am going to produce one more patch set for this CL to address a nit from phoglund@ and remove validation of droppedFramesFPS because as emircan@ pointed out, droppedFramesFPS is unrelated to canvas capture. I will still log droppedFramesFPS counter. I also noticed that the test sometimes fails on MAC OS because of HW decode disabled, I need more info from emrican@ on that topic before I feel confident submitting this browser test.
Hi, I addresses the last nits in patch set 4
On 2016/02/17 22:18:19, cpaulin wrote: > Hi, > I addresses the last nits in patch set 4 Thanks. Still lgtm.
ship it
The CQ bit was checked by cpaulin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phoglund@chromium.org Link to the patchset: https://codereview.chromium.org/1692773002/#ps140001 (title: "Addressed emircan@'s comments and phoglund@'s comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692773002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692773002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #5 (id:160001) has been deleted
The CQ bit was checked by cpaulin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org, phoglund@chromium.org Link to the patchset: https://codereview.chromium.org/1692773002/#ps180001 (title: "Excluding mac trybots for now.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692773002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692773002/180001
Message was sent while issue was closed.
Description was changed from ========== Canvas Capture: chrome browsertest- added frame rate test This CL introduces a new chrome browser test for canvas capture. The test verifies that the canvas capture frame rate is as expected. BUG=524218, 586316 TEST=On linux host and trybots ========== to ========== Canvas Capture: chrome browsertest- added frame rate test This CL introduces a new chrome browser test for canvas capture. The test verifies that the canvas capture frame rate is as expected. BUG=524218, 586316 TEST=On linux host and trybots ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Canvas Capture: chrome browsertest- added frame rate test This CL introduces a new chrome browser test for canvas capture. The test verifies that the canvas capture frame rate is as expected. BUG=524218, 586316 TEST=On linux host and trybots ========== to ========== Canvas Capture: chrome browsertest- added frame rate test This CL introduces a new chrome browser test for canvas capture. The test verifies that the canvas capture frame rate is as expected. BUG=524218, 586316 TEST=On linux host and trybots Committed: https://crrev.com/7a05c134dedd0137c23ba43b42db8f1e513cd9e6 Cr-Commit-Position: refs/heads/master@{#379065} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7a05c134dedd0137c23ba43b42db8f1e513cd9e6 Cr-Commit-Position: refs/heads/master@{#379065}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:180001) has been created in https://codereview.chromium.org/1762703004/ by benwells@chromium.org. The reason for reverting is: This CL caused uninitialized memory access errors in the new test. See here for the first build where this occurred: https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20ChromeOS%20... here is some sample output: WebRtcCanvasCaptureBrowserTest.VerifyCanvasCaptureFrameRate (run #1): [ RUN ] WebRtcCanvasCaptureBrowserTest.VerifyCanvasCaptureFrameRate [29378:29378:0303/133319:WARNING:chrome_browser_main_chromeos.cc(336)] Running as stub user with profile dir: test-user [29378:29378:0303/133319:WARNING:statistics_provider.cc(251)] Statistics loaded after waiting 2ms. Xlib: extension "RANDR" missing on display ":9". [29378:29378:0303/133319:INFO:lock_state_controller.cc(95)] Constructing LockStateController instance 0x72a00000e600 [29378:29465:0303/133319:WARNING:local_extension_cache.cc(259)] Extensions will not be installed from update URLs until /tmp/.org.chromium.Chromium.bKaPXe/ddd7Mcg/stub_device_local_extension_cache/.initialized exists. [29378:29378:0303/133320:WARNING:child_account_service.cc(289)] User instance wasn't found while setting child account flag. [29378:29469:0303/133320:WARNING:freezer_cgroup_process_manager.cc(59)] Cgroup freezer does not exist or is not writable. Unable to freeze renderer processes. [29378:29378:0303/133320:INFO:power_button_observer.cc(37)] Creating PowerButtonObserver 0x7060001e9ae0 [29378:29378:0303/133320:INFO:lock_state_controller.cc(235)] LockStateController::OnLoginStateChanged login_status_: 0, status: 2 [29378:29378:0303/133320:WARNING:suggestions_service.cc(155)] Token error: Not authorized. Uninitialized bytes in __interceptor_strlen at offset 20 inside [0x73e00000ee00, 3280) ==29483==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x7f8620d25de8 in ?? ??:0 #1 0x7f8620d2b3f8 in ?? ??:0 #2 0x7f8620d27a17 in ?? ??:0 #3 0x7f8620d27c55 in ?? ??:0 #4 0xb07a8e2 in glXQueryExtensionsStringFn ui/gl/gl_glx_api_implementation.cc:91:21 #5 0xb1470ff in GetPlatformExtensions ui/gl/gl_bindings.cc:59:21 #6 0xb13fb9c in InitializeExtensionBindings ui/gl/gl_bindings_autogen_glx.cc:118:26 #7 0xb058ed0 in InitializeStaticGLBindings ui/gl/gl_implementation_x11.cc:102:7 #8 0xb1153c1 in ?? ui/gl/gl_surface.cc:75:7 #9 0xb114e69 in InitializeOneOff ui/gl/gl_surface.cc:65:10 #10 0x1eaeaf86 in GpuMain content/gpu/gpu_main.cc:295:15 #11 0x1a310e81 in RunNamedProcessTypeMain content/app/content_main_runner.cc:395:14 #12 0x1a31422a in Run content/app/content_main_runner.cc:766:12 #13 0x1a30ca37 in ContentMain content/app/content_main.cc:19:15 #14 0x1a3e5050 in LaunchTests content/public/test/test_launcher.cc:505:12 #15 0x5680cbf in LaunchChromeTests chrome/test/base/chrome_test_launcher.cc:128:10 #16 0x3167cec in main chrome/test/base/browser_tests_main.cc:21:10 #17 0x7f862401a76c in __libc_start_main /build/eglibc-rrybNj/eglibc-2.15/csu/libc-start.c:226:0 #18 0x76ae88 in _start ??:0 Uninitialized value was created by a heap allocation #0 0x78f8c2 in malloc ??:0 #1 0x7f8620d2b972 in ?? ??:0 SUMMARY: MemorySanitizer: use-of-uninitialized-value (/usr/lib/x86_64-linux-gnu/mesa/libGL.so.1+0x1ade8) Exiting .
Message was sent while issue was closed.
On 2016/03/04 02:32:03, benwells wrote: > A revert of this CL (patchset #5 id:180001) has been created in > https://codereview.chromium.org/1762703004/ by mailto:benwells@chromium.org. > > The reason for reverting is: This CL caused uninitialized memory access errors > in the new test. See here for the first build where this occurred: > https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20ChromeOS%20... > > here is some sample output: > WebRtcCanvasCaptureBrowserTest.VerifyCanvasCaptureFrameRate (run #1): > [ RUN ] WebRtcCanvasCaptureBrowserTest.VerifyCanvasCaptureFrameRate > [29378:29378:0303/133319:WARNING:chrome_browser_main_chromeos.cc(336)] Running > as stub user with profile dir: test-user > [29378:29378:0303/133319:WARNING:statistics_provider.cc(251)] Statistics loaded > after waiting 2ms. > Xlib: extension "RANDR" missing on display ":9". > [29378:29378:0303/133319:INFO:lock_state_controller.cc(95)] Constructing > LockStateController instance 0x72a00000e600 > [29378:29465:0303/133319:WARNING:local_extension_cache.cc(259)] Extensions will > not be installed from update URLs until > /tmp/.org.chromium.Chromium.bKaPXe/ddd7Mcg/stub_device_local_extension_cache/.initialized > exists. > [29378:29378:0303/133320:WARNING:child_account_service.cc(289)] User instance > wasn't found while setting child account flag. > [29378:29469:0303/133320:WARNING:freezer_cgroup_process_manager.cc(59)] Cgroup > freezer does not exist or is not writable. Unable to freeze renderer processes. > [29378:29378:0303/133320:INFO:power_button_observer.cc(37)] Creating > PowerButtonObserver 0x7060001e9ae0 > [29378:29378:0303/133320:INFO:lock_state_controller.cc(235)] > LockStateController::OnLoginStateChanged login_status_: 0, status: 2 > [29378:29378:0303/133320:WARNING:suggestions_service.cc(155)] Token error: Not > authorized. > Uninitialized bytes in __interceptor_strlen at offset 20 inside [0x73e00000ee00, > 3280) > ==29483==WARNING: MemorySanitizer: use-of-uninitialized-value > #0 0x7f8620d25de8 in ?? ??:0 > #1 0x7f8620d2b3f8 in ?? ??:0 > #2 0x7f8620d27a17 in ?? ??:0 > #3 0x7f8620d27c55 in ?? ??:0 > #4 0xb07a8e2 in glXQueryExtensionsStringFn > ui/gl/gl_glx_api_implementation.cc:91:21 > #5 0xb1470ff in GetPlatformExtensions ui/gl/gl_bindings.cc:59:21 > #6 0xb13fb9c in InitializeExtensionBindings > ui/gl/gl_bindings_autogen_glx.cc:118:26 > #7 0xb058ed0 in InitializeStaticGLBindings > ui/gl/gl_implementation_x11.cc:102:7 > #8 0xb1153c1 in ?? ui/gl/gl_surface.cc:75:7 > #9 0xb114e69 in InitializeOneOff ui/gl/gl_surface.cc:65:10 > #10 0x1eaeaf86 in GpuMain content/gpu/gpu_main.cc:295:15 > #11 0x1a310e81 in RunNamedProcessTypeMain > content/app/content_main_runner.cc:395:14 > #12 0x1a31422a in Run content/app/content_main_runner.cc:766:12 > #13 0x1a30ca37 in ContentMain content/app/content_main.cc:19:15 > #14 0x1a3e5050 in LaunchTests content/public/test/test_launcher.cc:505:12 > #15 0x5680cbf in LaunchChromeTests > chrome/test/base/chrome_test_launcher.cc:128:10 > #16 0x3167cec in main chrome/test/base/browser_tests_main.cc:21:10 > #17 0x7f862401a76c in __libc_start_main > /build/eglibc-rrybNj/eglibc-2.15/csu/libc-start.c:226:0 > #18 0x76ae88 in _start ??:0 > > Uninitialized value was created by a heap allocation > #0 0x78f8c2 in malloc ??:0 > #1 0x7f8620d2b972 in ?? ??:0 > > SUMMARY: MemorySanitizer: use-of-uninitialized-value > (/usr/lib/x86_64-linux-gnu/mesa/libGL.so.1+0x1ade8) > Exiting > . cpaulin: after relanding this, would it make sense to also add a similar test that logs perf output for the frame rate? So we can track how it progresses over time?
Message was sent while issue was closed.
Interesting, I will try to reproduce On Fri, Mar 4, 2016 at 5:01 AM, <kjellander@chromium.org> wrote: > On 2016/03/04 02:32:03, benwells wrote: > > A revert of this CL (patchset #5 id:180001) has been created in > > https://codereview.chromium.org/1762703004/ by mailto: > benwells@chromium.org. > > > > > The reason for reverting is: This CL caused uninitialized memory access > errors > > in the new test. See here for the first build where this occurred: > > > > https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20ChromeOS%20... > > > > here is some sample output: > > WebRtcCanvasCaptureBrowserTest.VerifyCanvasCaptureFrameRate (run #1): > > [ RUN ] WebRtcCanvasCaptureBrowserTest.VerifyCanvasCaptureFrameRate > > [29378:29378:0303/133319:WARNING:chrome_browser_main_chromeos.cc(336)] > Running > > as stub user with profile dir: test-user > > [29378:29378:0303/133319:WARNING:statistics_provider.cc(251)] Statistics > loaded > > after waiting 2ms. > > Xlib: extension "RANDR" missing on display ":9". > > [29378:29378:0303/133319:INFO:lock_state_controller.cc(95)] Constructing > > LockStateController instance 0x72a00000e600 > > [29378:29465:0303/133319:WARNING:local_extension_cache.cc(259)] > Extensions > will > > not be installed from update URLs until > > > > /tmp/.org.chromium.Chromium.bKaPXe/ddd7Mcg/stub_device_local_extension_cache/.initialized > > exists. > > [29378:29378:0303/133320:WARNING:child_account_service.cc(289)] User > instance > > wasn't found while setting child account flag. > > [29378:29469:0303/133320:WARNING:freezer_cgroup_process_manager.cc(59)] > Cgroup > > freezer does not exist or is not writable. Unable to freeze renderer > processes. > > [29378:29378:0303/133320:INFO:power_button_observer.cc(37)] Creating > > PowerButtonObserver 0x7060001e9ae0 > > [29378:29378:0303/133320:INFO:lock_state_controller.cc(235)] > > LockStateController::OnLoginStateChanged login_status_: 0, status: 2 > > [29378:29378:0303/133320:WARNING:suggestions_service.cc(155)] Token > error: Not > > authorized. > > Uninitialized bytes in __interceptor_strlen at offset 20 inside > [0x73e00000ee00, > > 3280) > > ==29483==WARNING: MemorySanitizer: use-of-uninitialized-value > > #0 0x7f8620d25de8 in ?? ??:0 > > #1 0x7f8620d2b3f8 in ?? ??:0 > > #2 0x7f8620d27a17 in ?? ??:0 > > #3 0x7f8620d27c55 in ?? ??:0 > > #4 0xb07a8e2 in glXQueryExtensionsStringFn > > ui/gl/gl_glx_api_implementation.cc:91:21 > > #5 0xb1470ff in GetPlatformExtensions ui/gl/gl_bindings.cc:59:21 > > #6 0xb13fb9c in InitializeExtensionBindings > > ui/gl/gl_bindings_autogen_glx.cc:118:26 > > #7 0xb058ed0 in InitializeStaticGLBindings > > ui/gl/gl_implementation_x11.cc:102:7 > > #8 0xb1153c1 in ?? ui/gl/gl_surface.cc:75:7 > > #9 0xb114e69 in InitializeOneOff ui/gl/gl_surface.cc:65:10 > > #10 0x1eaeaf86 in GpuMain content/gpu/gpu_main.cc:295:15 > > #11 0x1a310e81 in RunNamedProcessTypeMain > > content/app/content_main_runner.cc:395:14 > > #12 0x1a31422a in Run content/app/content_main_runner.cc:766:12 > > #13 0x1a30ca37 in ContentMain content/app/content_main.cc:19:15 > > #14 0x1a3e5050 in LaunchTests content/public/test/test_launcher.cc:505:12 > > #15 0x5680cbf in LaunchChromeTests > > chrome/test/base/chrome_test_launcher.cc:128:10 > > #16 0x3167cec in main chrome/test/base/browser_tests_main.cc:21:10 > > #17 0x7f862401a76c in __libc_start_main > > /build/eglibc-rrybNj/eglibc-2.15/csu/libc-start.c:226:0 > > #18 0x76ae88 in _start ??:0 > > > > Uninitialized value was created by a heap allocation > > #0 0x78f8c2 in malloc ??:0 > > #1 0x7f8620d2b972 in ?? ??:0 > > > > SUMMARY: MemorySanitizer: use-of-uninitialized-value > > (/usr/lib/x86_64-linux-gnu/mesa/libGL.so.1+0x1ade8) > > Exiting > > . > > cpaulin: after relanding this, would it make sense to also add a similar > test > that logs perf output for the frame rate? So we can track how it > progresses over > time? > > https://codereview.chromium.org/1692773002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
kjellander@ - what if the value is correct but moves up/down over time, what would that tell you? On Fri, Mar 4, 2016 at 12:28 PM, Christian Paulin <cpaulin@google.com> wrote: > Interesting, I will try to reproduce > > On Fri, Mar 4, 2016 at 5:01 AM, <kjellander@chromium.org> wrote: > >> On 2016/03/04 02:32:03, benwells wrote: >> > A revert of this CL (patchset #5 id:180001) has been created in >> > https://codereview.chromium.org/1762703004/ by mailto: >> benwells@chromium.org. >> >> > >> > The reason for reverting is: This CL caused uninitialized memory access >> errors >> > in the new test. See here for the first build where this occurred: >> > >> >> https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20ChromeOS%20... >> > >> > here is some sample output: >> > WebRtcCanvasCaptureBrowserTest.VerifyCanvasCaptureFrameRate (run #1): >> > [ RUN ] WebRtcCanvasCaptureBrowserTest.VerifyCanvasCaptureFrameRate >> > [29378:29378:0303/133319:WARNING:chrome_browser_main_chromeos.cc(336)] >> Running >> > as stub user with profile dir: test-user >> > [29378:29378:0303/133319:WARNING:statistics_provider.cc(251)] Statistics >> loaded >> > after waiting 2ms. >> > Xlib: extension "RANDR" missing on display ":9". >> > [29378:29378:0303/133319:INFO:lock_state_controller.cc(95)] Constructing >> > LockStateController instance 0x72a00000e600 >> > [29378:29465:0303/133319:WARNING:local_extension_cache.cc(259)] >> Extensions >> will >> > not be installed from update URLs until >> > >> >> /tmp/.org.chromium.Chromium.bKaPXe/ddd7Mcg/stub_device_local_extension_cache/.initialized >> > exists. >> > [29378:29378:0303/133320:WARNING:child_account_service.cc(289)] User >> instance >> > wasn't found while setting child account flag. >> > [29378:29469:0303/133320:WARNING:freezer_cgroup_process_manager.cc(59)] >> Cgroup >> > freezer does not exist or is not writable. Unable to freeze renderer >> processes. >> > [29378:29378:0303/133320:INFO:power_button_observer.cc(37)] Creating >> > PowerButtonObserver 0x7060001e9ae0 >> > [29378:29378:0303/133320:INFO:lock_state_controller.cc(235)] >> > LockStateController::OnLoginStateChanged login_status_: 0, status: 2 >> > [29378:29378:0303/133320:WARNING:suggestions_service.cc(155)] Token >> error: Not >> > authorized. >> > Uninitialized bytes in __interceptor_strlen at offset 20 inside >> [0x73e00000ee00, >> > 3280) >> > ==29483==WARNING: MemorySanitizer: use-of-uninitialized-value >> > #0 0x7f8620d25de8 in ?? ??:0 >> > #1 0x7f8620d2b3f8 in ?? ??:0 >> > #2 0x7f8620d27a17 in ?? ??:0 >> > #3 0x7f8620d27c55 in ?? ??:0 >> > #4 0xb07a8e2 in glXQueryExtensionsStringFn >> > ui/gl/gl_glx_api_implementation.cc:91:21 >> > #5 0xb1470ff in GetPlatformExtensions ui/gl/gl_bindings.cc:59:21 >> > #6 0xb13fb9c in InitializeExtensionBindings >> > ui/gl/gl_bindings_autogen_glx.cc:118:26 >> > #7 0xb058ed0 in InitializeStaticGLBindings >> > ui/gl/gl_implementation_x11.cc:102:7 >> > #8 0xb1153c1 in ?? ui/gl/gl_surface.cc:75:7 >> > #9 0xb114e69 in InitializeOneOff ui/gl/gl_surface.cc:65:10 >> > #10 0x1eaeaf86 in GpuMain content/gpu/gpu_main.cc:295:15 >> > #11 0x1a310e81 in RunNamedProcessTypeMain >> > content/app/content_main_runner.cc:395:14 >> > #12 0x1a31422a in Run content/app/content_main_runner.cc:766:12 >> > #13 0x1a30ca37 in ContentMain content/app/content_main.cc:19:15 >> > #14 0x1a3e5050 in LaunchTests >> content/public/test/test_launcher.cc:505:12 >> > #15 0x5680cbf in LaunchChromeTests >> > chrome/test/base/chrome_test_launcher.cc:128:10 >> > #16 0x3167cec in main chrome/test/base/browser_tests_main.cc:21:10 >> > #17 0x7f862401a76c in __libc_start_main >> > /build/eglibc-rrybNj/eglibc-2.15/csu/libc-start.c:226:0 >> > #18 0x76ae88 in _start ??:0 >> > >> > Uninitialized value was created by a heap allocation >> > #0 0x78f8c2 in malloc ??:0 >> > #1 0x7f8620d2b972 in ?? ??:0 >> > >> > SUMMARY: MemorySanitizer: use-of-uninitialized-value >> > (/usr/lib/x86_64-linux-gnu/mesa/libGL.so.1+0x1ade8) >> > Exiting >> > . >> >> cpaulin: after relanding this, would it make sense to also add a similar >> test >> that logs perf output for the frame rate? So we can track how it >> progresses over >> time? >> >> https://codereview.chromium.org/1692773002/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
mcasas@chromium.org changed reviewers: + mcasas@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1692773002/diff/180001/chrome/browser/media/c... File chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc (right): https://codereview.chromium.org/1692773002/diff/180001/chrome/browser/media/c... chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc:16: #include "content/public/browser/render_process_host.h" I'm not sure all these includes are needed. There's no reference to RenderProcessHost for example. Perhaps taken from another test? https://codereview.chromium.org/1692773002/diff/180001/chrome/browser/media/c... chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc:36: // At this time this browser test verifies that the frame rate of a stream I'm surprised to read "At this time". Maybe you could rephrase as // Verifies the frame of a Media Stream created out of a <canvas>. // TODO(cpaulin): .Extend this test to... https://codereview.chromium.org/1692773002/diff/180001/chrome/browser/media/c... chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc:38: class MAYBE_WebRtcCanvasCaptureBrowserTest : public InProcessBrowserTest { The name of the test does not match the file name. This is also true of other files in this folder, so I made a CL to clean that up: http://crrev.com/1762413002 https://codereview.chromium.org/1692773002/diff/180001/chrome/browser/media/c... chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc:43: command_line->AppendSwitch(switches::kUseGpuInTests); Someone else says: // This test enables switches::kUseGpuInTests which causes false positives // with MemorySanitizer. in https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ... which could be the issue. https://codereview.chromium.org/1692773002/diff/180001/chrome/browser/media/c... chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc:60: } I was thinking this looks strangely indented and rightly so: std::string ExecuteJavascript(const std::string& javascript, content::WebContents* tab_contents) const { std::string result; EXPECT_TRUE(content::ExecuteScriptAndExtractString(tab_contents, javascript, &result)); return result; } is correct. I strongly suggest you run git cl format before uploading your patch. https://codereview.chromium.org/1692773002/diff/180001/chrome/browser/media/c... chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc:63: DISALLOW_COPY_AND_ASSIGN(MAYBE_WebRtcCanvasCaptureBrowserTest); nit: this is one space too far right. https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... File chrome/test/data/media/canvas_capture_test.html (right): https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:1: <!DOCTYPE html> This file should go into chrome/test/data/media/html, don't you think? Why isn't in chrome/test/data/webrtc? And someone should move the webms and mp4s of chrome/test/data/media into chrome/test/data/media/resources. https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:9: <source src='bear-640x360.webm' type='video/webm'> What's the problem with the other bear webms around https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/l... ? https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:13: <canvas id='canvas' width=640 height=360></canvas> Large canvas, for a browsertests that is supposed to verify functionality. Larger canvases mean slower tests, what about 320x240? Or 160x120? https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:22: var TOLERANCE_PERCENT = 15 / 100; This is not a percentage, but a per_one in any case :) Just call it tolerance. https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:45: this.theCanvasStream = null; Inconsistent indenting: here you use 2 spaces whereas in drat() and onload (for example) you are using 4. https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:57: var test = this; Why aliasing? In l. 100+ you use |this|. Const? https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:58: test.targetFPS = frameRate ? frameRate : DEFAULT_FRAME_RATE; This function is only called by testFrameRateOfCanvasCapture() and that in turn is only called with no arguments so this line is superfluous. https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:74: if (this.decodedFPS.length == 6) { Make a const out of this, put it on top of file? https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:80: return; Uncertain about this but: {} around? (due to multiline condition) https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:121: for (var i = 0; i < count; i++) { for (value i in array) ... https://google.github.io/styleguide/javascriptguide.xml#for-in_loop
Message was sent while issue was closed.
This CL is closed, trying to reland it in https://codereview.chromium.org/1764983003/ I addressed comments in new CL and disabled MSAN/ASAN Thanks Christian https://codereview.chromium.org/1692773002/diff/180001/chrome/browser/media/c... File chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc (right): https://codereview.chromium.org/1692773002/diff/180001/chrome/browser/media/c... chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc:16: #include "content/public/browser/render_process_host.h" On 2016/03/04 22:30:33, mcasas wrote: > I'm not sure all these includes are needed. There's > no reference to RenderProcessHost for example. > Perhaps taken from another test? I pruned the not needed includes. Done. https://codereview.chromium.org/1692773002/diff/180001/chrome/browser/media/c... chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc:36: // At this time this browser test verifies that the frame rate of a stream On 2016/03/04 22:30:33, mcasas wrote: > I'm surprised to read "At this time". Maybe you could > rephrase as > > // Verifies the frame of a Media Stream created out of a <canvas>. > // TODO(cpaulin): .Extend this test to... That's fine, this was indeed not necessary info. Done https://codereview.chromium.org/1692773002/diff/180001/chrome/browser/media/c... chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc:38: class MAYBE_WebRtcCanvasCaptureBrowserTest : public InProcessBrowserTest { On 2016/03/04 22:30:33, mcasas wrote: > The name of the test does not match the file name. > This is also true of other files in this folder, so I > made a CL to clean that up: http://crrev.com/1762413002 OK, I follow your example CL now. Done https://codereview.chromium.org/1692773002/diff/180001/chrome/browser/media/c... chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc:43: command_line->AppendSwitch(switches::kUseGpuInTests); On 2016/03/04 22:30:33, mcasas wrote: > Someone else says: > // This test enables switches::kUseGpuInTests which causes false positives > // with MemorySanitizer. > > in > https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ... > which could be the issue. Thanks for the heads up. Done. https://codereview.chromium.org/1692773002/diff/180001/chrome/browser/media/c... chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc:60: } On 2016/03/04 22:30:33, mcasas wrote: > I was thinking this looks strangely indented and rightly so: > > std::string ExecuteJavascript(const std::string& javascript, > content::WebContents* tab_contents) const { > std::string result; > EXPECT_TRUE(content::ExecuteScriptAndExtractString(tab_contents, javascript, > &result)); > return result; > } > > > is correct. > > I strongly suggest you run > git cl format > before uploading your patch. Done. https://codereview.chromium.org/1692773002/diff/180001/chrome/browser/media/c... chrome/browser/media/chrome_webrtc_canvas_capture_browsertest.cc:63: DISALLOW_COPY_AND_ASSIGN(MAYBE_WebRtcCanvasCaptureBrowserTest); On 2016/03/04 22:30:33, mcasas wrote: > nit: this is one space too far right. Done. https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... File chrome/test/data/media/canvas_capture_test.html (right): https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:1: <!DOCTYPE html> On 2016/03/04 22:30:33, mcasas wrote: > This file should go into > chrome/test/data/media/html, don't you think? > > Why isn't in chrome/test/data/webrtc? > > And someone should move > the webms and mp4s of chrome/test/data/media > into chrome/test/data/media/resources. I have the feeling chrome/test/data/webrtc is where the webrtc audio/video quality test lives, not sure if we should put it there. On the other hand chrome/test/data/media/html would be a good spot so that's where I am moving it to. https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:9: <source src='bear-640x360.webm' type='video/webm'> On 2016/03/04 22:30:33, mcasas wrote: > What's the problem with the other bear webms around > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/l... > ? They don't work, I tried them on linux and mac. What I mean is that they don't output any visible video, don't know why. https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:13: <canvas id='canvas' width=640 height=360></canvas> On 2016/03/04 22:30:33, mcasas wrote: > Large canvas, for a browsertests that is supposed > to verify functionality. Larger canvases mean slower > tests, what about 320x240? Or 160x120? I will look for a smaller video. Done. https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:22: var TOLERANCE_PERCENT = 15 / 100; On 2016/03/04 22:30:33, mcasas wrote: > This is not a percentage, but a per_one in > any case :) > Just call it tolerance. Done. https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:45: this.theCanvasStream = null; On 2016/03/04 22:30:33, mcasas wrote: > Inconsistent indenting: here you use 2 spaces > whereas in drat() and onload (for example) you > are using 4. window.onload indent is a line break not a typical function declaration and accordingly the line break requires 4 spaces. But you are right that function draw() {...} had wacko indents. Done https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:57: var test = this; On 2016/03/04 22:30:33, mcasas wrote: > Why aliasing? In l. 100+ you use |this|. > Const? Reason is this would not work: testVideo.addEventListener('play', function(event) { this.startTime = new Date().getTime(); this.interval = setInterval(this.calculateStats.bind(this, testVideo), 1000); }); Because 'this' means different things to different objects. I am taker of a better solution if any. Done. https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:58: test.targetFPS = frameRate ? frameRate : DEFAULT_FRAME_RATE; On 2016/03/04 22:30:33, mcasas wrote: > This function is only called by > testFrameRateOfCanvasCapture() > and that in turn is only called with > no arguments so this line is superfluous. I could remove the argument, it is not used in this test. Done. https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:74: if (this.decodedFPS.length == 6) { On 2016/03/04 22:30:33, mcasas wrote: > Make a const out of this, put it on top of file? Done. https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:80: return; On 2016/03/04 22:30:33, mcasas wrote: > Uncertain about this but: {} around? > (due to multiline condition) Done. https://codereview.chromium.org/1692773002/diff/180001/chrome/test/data/media... chrome/test/data/media/canvas_capture_test.html:121: for (var i = 0; i < count; i++) { On 2016/03/04 22:30:33, mcasas wrote: > for (value i in array) > ... > > https://google.github.io/styleguide/javascriptguide.xml#for-in_loop Done.
Message was sent while issue was closed.
Description was changed from ========== Canvas Capture: chrome browsertest- added frame rate test This CL introduces a new chrome browser test for canvas capture. The test verifies that the canvas capture frame rate is as expected. BUG=524218, 586316 TEST=On linux host and trybots Committed: https://crrev.com/7a05c134dedd0137c23ba43b42db8f1e513cd9e6 Cr-Commit-Position: refs/heads/master@{#379065} ========== to ========== Canvas Capture: chrome browsertest- added frame rate test This CL introduces a new chrome browser test for canvas capture. The test verifies that the canvas capture frame rate is as expected. BUG=524218, 586316 TEST=On linux host and trybots Committed: https://crrev.com/7a05c134dedd0137c23ba43b42db8f1e513cd9e6 Cr-Commit-Position: refs/heads/master@{#379065} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
