New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 660900 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jan 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

74.7% regression in media.tough_video_cases_extra at 428149:428230

Project Member Reported by dalecur...@chromium.org, Oct 31 2016

Issue description

Seems hubbe@'s change to increase video threads significantly delays frames in the OGV case.

https://chromium.googlesource.com/chromium/src/+/c99cc371fad707aed0f0e3d90b889be24e907b91
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=660900

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


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

chromium-rel-win7-gpu-intel

Comment 3 by hubbe@chromium.org, Oct 31 2016

It makes total sense for this to be caused by my CL.

However, I'm not sure how much stock to put in this test as we can't seem to play back this video (tools/perf/page_sets/tough_video_cases/garden2_10s.ogv) smoothly, with or without my patch. I had hoped that adding additional threads would help, but it would seem that the theora codec just doesn't use the extra threads.

The simplest fix is probably to update the code so that it only increases the number of threads for codecs that actually support threading, like h264.

Does anybody actually use theora though?

Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Oct 31 2016

Cc: hubbe@chromium.org

=== Auto-CCing suspected CL author hubbe@chromium.org ===

Hi hubbe@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 : scale video threads with resolution
Author  : hubbe
Commit description:
  
This allows 4k video to play smoothly on high-end machines, even when software-decoded.

Most machines today have 2-8 execution contexts, using
more cores generally doesn't seem to increase power usage
and allows us to decode video faster.

The number of threads we use proportional to the number of pixels, normalized so that 1080p gets three threads.

Examples:
 4k: 12 threads
 1440p: 5 threads
 1080p: 3 threads
 anything lower than 1080p: 2 threads

BUG=

Review-Url: https://codereview.chromium.org/2448453004
Cr-Commit-Position: refs/heads/master@{#428214}
Commit  : c99cc371fad707aed0f0e3d90b889be24e907b91
Date    : Thu Oct 27 23:39:24 2016


===== TESTED REVISIONS =====
Revision         Mean    Std Dev  N  Good?
chromium@428148  72.905  3.2304   5  good
chromium@428189  72.718  2.61563  5  good
chromium@428210  72.243  1.09492  5  good
chromium@428213  71.832  1.37685  5  good
chromium@428214  86.022  2.38176  5  bad    <--
chromium@428215  86.108  3.37124  5  bad
chromium@428220  87.524  4.03359  5  bad
chromium@428230  84.323  2.17932  5  bad

Bisect job ran on: winx64intel_perf_bisect
Bug ID: 660900

Test Command: src/tools/perf/run_benchmark -v --browser=release_x64 --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests media.tough_video_cases_extra
Test Metric: seek/garden2_10s.ogv_seek_cold
Relative Change: 15.66%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/winx64intel_perf_bisect/builds/1221
Job details: https://chromeperf.appspot.com/buildbucket_job_status/8997265222347286688


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5861864116846592

| 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!
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d4cc08f819e66335d32a72bd500781d2d36be99f

commit d4cc08f819e66335d32a72bd500781d2d36be99f
Author: hubbe <hubbe@chromium.org>
Date: Tue Nov 01 05:06:09 2016

media: Only add more threads for codecs that benefit from it

Adding more threads tends to increase seek time. For high resolution
videos this is probably a reasonable tradeoff as we get smoother
playback. However, some codecs don't use threads for decoding at all,
and for those codecs adding more threads does nothing except increase
decoding latency and seek time.

BUG= 660900 

Review-Url: https://codereview.chromium.org/2467623002
Cr-Commit-Position: refs/heads/master@{#428940}

[modify] https://crrev.com/d4cc08f819e66335d32a72bd500781d2d36be99f/media/filters/ffmpeg_video_decoder.cc

Status: Fixed (was: Assigned)
Fixed in graphs we have data for w/ the above patch.
Status: Assigned (was: Fixed)
Looks like the fix regressed ogv playback for "warm" seeks, but fixed "cold" seeks! I believe that warm vs cold is cached vs not.
https://chromeperf.appspot.com/report?sid=ac033814bd3d167d7d2f48b41782733ce28563c2b7ec29918f616d6fcfd63147&rev=428942


Also here's the graph for energy changes:
https://chromeperf.appspot.com/report?sid=6f3342f38b91c48b4227252449723bc85192f044dfd5c9dc5c5994978d31ec1f&rev=428234

Comment 8 by hubbe@chromium.org, Nov 4 2016

There is something odd with this OGV clip though.
On the second playthrough (with lots of threads) the whole clip plays in like ~2 seconds...

Haha, that doesn't seem right. Does remuxing the clip help?
Remuxing seems to help, although avconv complains bitterly, saying:

Non-monotonous DTS in output stream 0:0; previous: 298, current: 245; changing to 299. This may result in incorrect timestamps in the output file.

So I'm not sure the stream is actually "fixed".
Also, the clip seems to be 2 seconds longer after remuxing it.
Probably we should just drop this clip since no one uses 1080p/4k theora.
Labels: Performance
Status: Fixed (was: Assigned)
I'm marking this Fixed since it seems like the important part of the bug was fixed. 

Sign in to add a comment