New issue
Advanced search Search tips

Issue 602759 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

10.9% regression in webrtc.peerconnection at 386068:386124

Project Member Reported by m...@chromium.org, Apr 12 2016

Issue description

Comment 1 by m...@chromium.org, Apr 12 2016

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=602759

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICguILR7QoM


Bot(s) for this bug's original alert(s):

android-galaxy-s5
Project Member

Comment 2 by 42576172...@developer.gserviceaccount.com, Apr 12 2016

Cc: liber...@chromium.org
Owner: liber...@chromium.org

=== 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!
Labels: TE-Triaged
@liberato: Hey, would you mind checking the above issue as per comment#2 ?

I really appreciate your help.

Thank you!
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.
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.
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)
seems like it's back to normal.

Sign in to add a comment