Issue metadata
Sign in to add a comment
|
10.9% regression in webrtc.peerconnection at 386068:386124 |
||||||||||||||||||||
Issue description
,
Apr 12 2016
=== Auto-CCing suspected CL author liberato@chromium.org === Hi liberato@chromium.org, the bisect results pointed to your CL below as possibly causing a regression. Please have a look at this info and see whether your CL be related. ===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : Enable adaptive playback for spitzer, use conservative size. Author : liberato Commit description: Android MediaCodec allocates memory for adaptive playback (dynamic resolution change) based on hints provided to the codec during construction about the maximum expected decoded frame dimensions. That can signficantly increate the memory usage / reduce the number of concurrent codec instances that chrome can use. On some devices (Samsung S3), adaptive playback also entirely breaks decoding. Decoded frames can be tiled / black and white / generally quite incorrect. Spitzer had previously turned off dynamic resolution support to fix both of these. However, it turns out that enabling adaptive support is necessary: - returning unused decoded buffers to MediaCodec without rendering can incorrectly stop MediaCodec from decoding more frames without adaptive playback enabled. - some videos on some devices (N7) play very slowly otherwise. This CL requests adaptive playback when AVDA creates the MediaCodec instance. It also plumbs the inital coded size to AVDA, and send it to MediaCodecBridge. MediaCodecBridge now uses this size, rather than 1920x1080, as the max adaptive size hint to save memory. For devices like the S3, MediaCodecBridge ignores the request for adaptive playback, and keeps it off. Providing the size to MediaCodec will also allow it to choose an appropriate input buffer size. Previously, it used the requested size of 320x240 when estimating the size of an encoded frame. BUG=599908, 596211 , 601066 Review URL: https://codereview.chromium.org/1869103002 Cr-Commit-Position: refs/heads/master@{#386091} Commit : 62cf4f1fab4e0cc2560983465d3fd15fde537f00 Date : Fri Apr 08 16:08:10 2016 ===== TESTED REVISIONS ===== Revision Mean Value Std. Dev. Num Values Good? chromium@386067 36105.6 230.921632 5 good chromium@386082 36418.4 209.84947 5 good chromium@386089 36148.8 343.434419 5 good chromium@386090 36176.8 233.36495 5 good chromium@386091 39800.8 261.708234 5 bad <- chromium@386093 39916.8 204.59521 5 bad chromium@386096 39731.2 252.307749 5 bad chromium@386124 39695.2 348.372215 5 bad Bisect job ran on: android_s5_perf_bisect Bug ID: 602759 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --also-run-disabled-tests webrtc.peerconnection Test Metric: vm_private_dirty_final_browser/vm_private_dirty_final_browser Relative Change: 9.94% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_s5_perf_bisect/builds/594 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9015554077499642768 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=602759 | O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq | X | for more information addressing perf regression bugs. For feedback, | / \ | file a bug with component Tests>AutoBisect. Thank you!
,
Apr 19 2016
@liberato: Hey, would you mind checking the above issue as per comment#2 ? I really appreciate your help. Thank you!
,
Apr 21 2016
i've been ooo since last week. i'll add it to my "dig myself out of vacation" todo list. at first glance, i'm not surprised that the CL increases the amount of memory required. however, the savings might not be real, since the CL more or less reverts a change made some months ago in an effort to save memory. i'll see when that earlier change landed, and if the graph earlier than that was comparable to what it is now. if so, then it'll turn out not to really be a regression. the "improvement" in the middle actually breaks a lot of devices.
,
Apr 22 2016
the older change landed ~feb 23, but there's no corresponding decrease in vm_private_dirty_final_browser at that time. looks like my idea was wrong. one change that's introduced by my CL is that the expected default decoded size might be incorrect for webrtc (0,0) instead of the old default of (320,240). i will put together a change that puts it back to (320,240). we'll see if that also fixes this regression.
,
Apr 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/99ddcd4719b569522b5effad17de2125120ecd89 commit 99ddcd4719b569522b5effad17de2125120ecd89 Author: liberato <liberato@chromium.org> Date: Fri Apr 22 20:39:44 2016 Default VDA expected coded size to 320x240. Provide a default of 320x240 as the expected coded size when initializing VDA::Config. This was the default prior to https://codereview.chromium.org/1869103002 , but switched to 0x0 by that change. Any user of VDA except GpuVideoDecoder will use the default. BUG= 602759 Review URL: https://codereview.chromium.org/1915563002 Cr-Commit-Position: refs/heads/master@{#389228} [modify] https://crrev.com/99ddcd4719b569522b5effad17de2125120ecd89/media/video/video_decode_accelerator.h
,
Apr 25 2016
seems like it's back to normal. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by m...@chromium.org
, Apr 12 2016