Experiment for disabling accelerated video decoding.

Experiment for disabling accelerated video decoding.

BUG=442039
Committed: https://crrev.com/76ae6859d6f7a2bf19a440e15a64eb1fe39b4518
Cr-Commit-Position: refs/heads/master@{#328237}

[hubbe, 2015-05-01 00:26:48]

hubbe@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2015-05-01 06:12:00]

What does the finch side of this look like?

[hubbe, 2015-05-01 17:15:48]

On 2015/05/01 06:12:00, DaleCurtis wrote:
> What does the finch side of this look like?

Like this I think:

/ Built using the Finch configuration builder (http://go/finch-config).
// Please use it for new configs.
{
  "author": "videostack-team@google.com",
  "description": "Disable HW accelerated video decode to see if it improves
success rates",
  "bug_id": 442039,
  "study": {
    "name": "DisableAcceleratedVideoDecode",
    "expiry_date": "2015/12/31 23:59",
    "min_version": "43.*",
    "consistency": "session",
    "channels": [
      "canary",
      "dev"
    ],
    "platforms": [
      "windows"
    ],
    "default_experiment_name": "Enabled",
    "experiments": [
      {
        "name": "DisabledByFlag",
        "forcing_flag": "disable-accelerated-video-decode",
        "existing_behavior": true
      },
      {
        "name": "Disabled",
        "probability_weight": 50,
        "existing_behavior": false
      },
      {
        "name": "Enabled",
        "probability_weight": 50,
        "existing_behavior": true
      }
    ]
  },
  "histograms": [
    "Media.PipelineStatus.AudioVideo.H264.HW",
    "Media.PipelineStatus.AudioVideo.H264.SW"
  ]
}

[DaleCurtis, 2015-05-02 17:37:09]

Awesome, lgtm % nits

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
content/browser/gpu/gpu_data_manager_impl_private.cc:738:
base::CommandLine::ForCurrentProcess()) &&
Extract command_line into a local variable?

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
content/browser/gpu/gpu_data_manager_impl_private.cc:880: const std::string
group_name = base::FieldTrialList::FindFullName(
Pair with if statement below.

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
content/browser/gpu/gpu_data_manager_impl_private.cc:891: }
Style for this file seems to be no {} for one line ifs.

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
File content/browser/gpu/gpu_data_manager_impl_private.h (right):

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
content/browser/gpu/gpu_data_manager_impl_private.h:93: base::CommandLine*
command_line) const;
const ?

[hubbe, 2015-05-04 19:16:21]

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
content/browser/gpu/gpu_data_manager_impl_private.cc:738:
base::CommandLine::ForCurrentProcess()) &&
On 2015/05/02 17:37:09, DaleCurtis wrote:
> Extract command_line into a local variable?

Done.

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
content/browser/gpu/gpu_data_manager_impl_private.cc:880: const std::string
group_name = base::FieldTrialList::FindFullName(
On 2015/05/02 17:37:09, DaleCurtis wrote:
> Pair with if statement below.

No, it needs to be called here as it affects how statistics is reported for this
experiment in UMA.

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
content/browser/gpu/gpu_data_manager_impl_private.cc:891: }
On 2015/05/02 17:37:09, DaleCurtis wrote:
> Style for this file seems to be no {} for one line ifs.

Done.

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
File content/browser/gpu/gpu_data_manager_impl_private.h (right):

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
content/browser/gpu/gpu_data_manager_impl_private.h:93: base::CommandLine*
command_line) const;
On 2015/05/02 17:37:09, DaleCurtis wrote:
> const ?

Done.

[DaleCurtis, 2015-05-04 19:38:17]

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
content/browser/gpu/gpu_data_manager_impl_private.cc:880: const std::string
group_name = base::FieldTrialList::FindFullName(
On 2015/05/04 19:16:21, hubbe wrote:
> On 2015/05/02 17:37:09, DaleCurtis wrote:
> > Pair with if statement below.
> 
> No, it needs to be called here as it affects how statistics is reported for
this
> experiment in UMA.

Can you add a comment for that, that's not obvious :)

[hubbe, 2015-05-04 20:46:15]

hubbe@chromium.org changed reviewers:
+ kbr@chromium.org

[hubbe, 2015-05-04 20:46:16]

Adding kbr@ for OWNERS approval.

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/1121653002/diff/1/content/browser/gpu/gpu_dat...
content/browser/gpu/gpu_data_manager_impl_private.cc:880: const std::string
group_name = base::FieldTrialList::FindFullName(
On 2015/05/04 19:38:17, DaleCurtis wrote:
> On 2015/05/04 19:16:21, hubbe wrote:
> > On 2015/05/02 17:37:09, DaleCurtis wrote:
> > > Pair with if statement below.
> > 
> > No, it needs to be called here as it affects how statistics is reported for
> this
> > experiment in UMA.
> 
> Can you add a comment for that, that's not obvious :)

Done.

[Ken Russell (switch to Gerrit), 2015-05-04 21:10:05]

I'm dubious that doing this experiment is going to inform any good decision.
Hardware-accelerated video decoding is a requirement in order to achieve
acceptable performance on large screens. The only decision this data will be
used for is whether to turn it off. I think we should instead try to gather
about:gpu (or UMA stats) from users reporting or experiencing the problem, and
figure out the root cause and solution.

https://codereview.chromium.org/1121653002/diff/60001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/1121653002/diff/60001/content/browser/gpu/gpu...
content/browser/gpu/gpu_data_manager_impl_private.cc:885: // statistics are
bucket correctly in all cases.
bucket -> bucketed

[DaleCurtis, 2015-05-04 21:12:03]

YT will correlate this data with playback errors and other issues as reported
via their pipeline.

[Ken Russell (switch to Gerrit), 2015-05-04 21:24:42]

On 2015/05/04 21:12:03, DaleCurtis wrote:
> YT will correlate this data with playback errors and other issues as reported
> via their pipeline.

Will you or your team commit to removing this experiment when the desired data
has been gathered?

[DaleCurtis, 2015-05-04 22:11:21]

On 2015/05/04 21:24:42, Ken Russell wrote:
> On 2015/05/04 21:12:03, DaleCurtis wrote:
> > YT will correlate this data with playback errors and other issues as
reported
> > via their pipeline.
> 
> Will you or your team commit to removing this experiment when the desired data
> has been gathered?

Absolutely. hubbe@ can you add a comment tying the FieldTrial check to the bug
attached?

[chromium-reviews, 2015-05-04 22:15:08]

Done.


On Mon, May 4, 2015 at 3:11 PM, <dalecurtis@chromium.org> wrote:

> On 2015/05/04 21:24:42, Ken Russell wrote:
>
>> On 2015/05/04 21:12:03, DaleCurtis wrote:
>> > YT will correlate this data with playback errors and other issues as
>>
> reported
>
>> > via their pipeline.
>>
>
>  Will you or your team commit to removing this experiment when the desired
>> data
>> has been gathered?
>>
>
> Absolutely. hubbe@ can you add a comment tying the FieldTrial check to
> the bug
> attached?
>
> https://codereview.chromium.org/1121653002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Russell (switch to Gerrit), 2015-05-04 22:51:33]

Thanks. LGTM

[hubbe, 2015-05-05 00:13:03]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2015-05-05 00:13:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1121653002/#ps100001
(title: "bug ID added in comment")

[commit-bot: I haz the power, 2015-05-05 00:14:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1121653002/100001

[commit-bot: I haz the power, 2015-05-05 00:35:36]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-05 00:36:34]

Patchset 6 (id:??) landed as
https://crrev.com/76ae6859d6f7a2bf19a440e15a64eb1fe39b4518
Cr-Commit-Position: refs/heads/master@{#328237}