media: Inform VideoBlit/MFTransform of video color information

This should a bunch of color decoding bugs on windows.
For DX9, this means using VideoProcessBlt instead of StretchRect()
(There is a fallback path for StretchRect, not sure if that is needed.)
This is controlled by "--enable-features=video-blit-color-accuracy" which defaults to off and will be enabled by an experiment.

BUG=655417, 650977, 576419, 576411
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/070e970240f13d29be8930f9316f39ace711a116
Cr-Commit-Position: refs/heads/master@{#435431}

[hubbe, 2016-11-15 19:10:27]

Description was changed from

==========
media: Use VideoProcessBlt on DX9

This should fix most range and color errors on windows 7.

BUG=655417, 650977, 576419, 576411
==========

to

==========
media: Use VideoProcessBlt on DX9

This should fix most range and color errors on windows 7.

BUG=655417, 650977, 576419, 576411
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[hubbe, 2016-11-15 19:11:58]

The CQ bit was checked by hubbe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 19:12:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hubbe, 2016-11-15 19:13:37]

hubbe@chromium.org changed reviewers:
+ ccameron@chromium.org, jbauman@chromium.org

[hubbe, 2016-11-15 19:13:38]

Next I plan to figure out how to do this for dx11.

[ccameron, 2016-11-15 19:24:00]

https://codereview.chromium.org/2503063002/diff/1/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2503063002/diff/1/ui/gfx/color_space.h#newcod...
ui/gfx/color_space.h:171: static ColorSpace FromSkColorSpace(const
sk_sp<SkColorSpace>& sk_color_space);
I think it would be better to have accessor functions to query these properties
from the ColorSpace:

DXVA2_VideoPrimaries GetDXVA2VideoPrimaries() const;
DXVA2_VideoTransferFunction GetDXVA2TransferFunction() const;

If there are concerns about pulling in too many system headers in this file, we
could create a helper class in a color_space_win.h.

[commit-bot: I haz the power, 2016-11-15 19:24:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 19:24:12]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[hubbe, 2016-11-16 18:29:05]

https://codereview.chromium.org/2503063002/diff/1/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2503063002/diff/1/ui/gfx/color_space.h#newcod...
ui/gfx/color_space.h:171: static ColorSpace FromSkColorSpace(const
sk_sp<SkColorSpace>& sk_color_space);
On 2016/11/15 19:24:00, ccameron wrote:
> I think it would be better to have accessor functions to query these
properties
> from the ColorSpace:
> 
> DXVA2_VideoPrimaries GetDXVA2VideoPrimaries() const;
> DXVA2_VideoTransferFunction GetDXVA2TransferFunction() const;
> 
> If there are concerns about pulling in too many system headers in this file,
we
> could create a helper class in a color_space_win.h.
> 

That seems like overkill to me.

I assume the reason for this is to make it easier to replace the implementation
in the future. However, it's just trading slightly more work now, for slightly
less work later.

It would be different if these functions would be used in more than one place.
However, in this case I think we have the opposite as we're going to need a
different enum translation for the dx11 case in DXVAVieoDecodeAccelerator.

[ccameron, 2016-11-16 21:38:52]

On 2016/11/16 18:29:05, hubbe wrote:
> https://codereview.chromium.org/2503063002/diff/1/ui/gfx/color_space.h
> File ui/gfx/color_space.h (right):
> 
>
https://codereview.chromium.org/2503063002/diff/1/ui/gfx/color_space.h#newcod...
> ui/gfx/color_space.h:171: static ColorSpace FromSkColorSpace(const
> sk_sp<SkColorSpace>& sk_color_space);
> On 2016/11/15 19:24:00, ccameron wrote:
> > I think it would be better to have accessor functions to query these
> properties
> > from the ColorSpace:
> > 
> > DXVA2_VideoPrimaries GetDXVA2VideoPrimaries() const;
> > DXVA2_VideoTransferFunction GetDXVA2TransferFunction() const;
> > 
> > If there are concerns about pulling in too many system headers in this file,
> we
> > could create a helper class in a color_space_win.h.
> > 
> 
> That seems like overkill to me.
> 
> I assume the reason for this is to make it easier to replace the
implementation
> in the future. However, it's just trading slightly more work now, for slightly
> less work later.
> 
> It would be different if these functions would be used in more than one place.
> However, in this case I think we have the opposite as we're going to need a
> different enum translation for the dx11 case in DXVAVieoDecodeAccelerator.

I feel pretty strongly that adding DXVA and DXVA2 accessors is the right thing
to do -- it's really not much work to add those helper functions.

If you feel very very strongly that you don't want to do that, then I guess it's
fine and I can lgtm it, but I still think it's the wrong approach.

[hubbe, 2016-11-16 22:01:54]

On 2016/11/16 21:38:52, ccameron wrote:
> On 2016/11/16 18:29:05, hubbe wrote:
> > https://codereview.chromium.org/2503063002/diff/1/ui/gfx/color_space.h
> > File ui/gfx/color_space.h (right):
> > 
> >
>
https://codereview.chromium.org/2503063002/diff/1/ui/gfx/color_space.h#newcod...
> > ui/gfx/color_space.h:171: static ColorSpace FromSkColorSpace(const
> > sk_sp<SkColorSpace>& sk_color_space);
> > On 2016/11/15 19:24:00, ccameron wrote:
> > > I think it would be better to have accessor functions to query these
> > properties
> > > from the ColorSpace:
> > > 
> > > DXVA2_VideoPrimaries GetDXVA2VideoPrimaries() const;
> > > DXVA2_VideoTransferFunction GetDXVA2TransferFunction() const;
> > > 
> > > If there are concerns about pulling in too many system headers in this
file,
> > we
> > > could create a helper class in a color_space_win.h.
> > > 
> > 
> > That seems like overkill to me.
> > 
> > I assume the reason for this is to make it easier to replace the
> implementation
> > in the future. However, it's just trading slightly more work now, for
slightly
> > less work later.
> > 
> > It would be different if these functions would be used in more than one
place.
> > However, in this case I think we have the opposite as we're going to need a
> > different enum translation for the dx11 case in DXVAVieoDecodeAccelerator.
> 
> I feel pretty strongly that adding DXVA and DXVA2 accessors is the right thing
> to do -- it's really not much work to add those helper functions.
> 
> If you feel very very strongly that you don't want to do that, then I guess
it's
> fine and I can lgtm it, but I still think it's the wrong approach.

It looks like MFT enums uses the same values as DXVA2, so if I write DXVA2
converters/accessors they
can be re-used for MFT (with a cast).

[hubbe, 2016-11-16 22:40:09]

On 2016/11/16 22:01:54, hubbe wrote:
> On 2016/11/16 21:38:52, ccameron wrote:
> > On 2016/11/16 18:29:05, hubbe wrote:
> > > https://codereview.chromium.org/2503063002/diff/1/ui/gfx/color_space.h
> > > File ui/gfx/color_space.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2503063002/diff/1/ui/gfx/color_space.h#newcod...
> > > ui/gfx/color_space.h:171: static ColorSpace FromSkColorSpace(const
> > > sk_sp<SkColorSpace>& sk_color_space);
> > > On 2016/11/15 19:24:00, ccameron wrote:
> > > > I think it would be better to have accessor functions to query these
> > > properties
> > > > from the ColorSpace:
> > > > 
> > > > DXVA2_VideoPrimaries GetDXVA2VideoPrimaries() const;
> > > > DXVA2_VideoTransferFunction GetDXVA2TransferFunction() const;
> > > > 
> > > > If there are concerns about pulling in too many system headers in this
> file,
> > > we
> > > > could create a helper class in a color_space_win.h.
> > > > 
> > > 
> > > That seems like overkill to me.
> > > 
> > > I assume the reason for this is to make it easier to replace the
> > implementation
> > > in the future. However, it's just trading slightly more work now, for
> slightly
> > > less work later.
> > > 
> > > It would be different if these functions would be used in more than one
> place.
> > > However, in this case I think we have the opposite as we're going to need
a
> > > different enum translation for the dx11 case in DXVAVieoDecodeAccelerator.
> > 
> > I feel pretty strongly that adding DXVA and DXVA2 accessors is the right
thing
> > to do -- it's really not much work to add those helper functions.
> > 
> > If you feel very very strongly that you don't want to do that, then I guess
> it's
> > fine and I can lgtm it, but I still think it's the wrong approach.
> 
> It looks like MFT enums uses the same values as DXVA2, so if I write DXVA2
> converters/accessors they
> can be re-used for MFT (with a cast).

I think I'll have to use a helper class, because including the required windows
headers in color_space.h seems to create a bunch of naming conflicts in other
parts
of the code...

[hubbe, 2016-11-17 00:53:37]

The CQ bit was checked by hubbe@chromium.org to run a CQ dry run

[hubbe, 2016-11-17 00:54:27]

On 2016/11/16 22:40:09, hubbe wrote:
> On 2016/11/16 22:01:54, hubbe wrote:
> > On 2016/11/16 21:38:52, ccameron wrote:
> > > On 2016/11/16 18:29:05, hubbe wrote:
> > > > https://codereview.chromium.org/2503063002/diff/1/ui/gfx/color_space.h
> > > > File ui/gfx/color_space.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2503063002/diff/1/ui/gfx/color_space.h#newcod...
> > > > ui/gfx/color_space.h:171: static ColorSpace FromSkColorSpace(const
> > > > sk_sp<SkColorSpace>& sk_color_space);
> > > > On 2016/11/15 19:24:00, ccameron wrote:
> > > > > I think it would be better to have accessor functions to query these
> > > > properties
> > > > > from the ColorSpace:
> > > > > 
> > > > > DXVA2_VideoPrimaries GetDXVA2VideoPrimaries() const;
> > > > > DXVA2_VideoTransferFunction GetDXVA2TransferFunction() const;
> > > > > 
> > > > > If there are concerns about pulling in too many system headers in this
> > file,
> > > > we
> > > > > could create a helper class in a color_space_win.h.
> > > > > 
> > > > 
> > > > That seems like overkill to me.
> > > > 
> > > > I assume the reason for this is to make it easier to replace the
> > > implementation
> > > > in the future. However, it's just trading slightly more work now, for
> > slightly
> > > > less work later.
> > > > 
> > > > It would be different if these functions would be used in more than one
> > place.
> > > > However, in this case I think we have the opposite as we're going to
need
> a
> > > > different enum translation for the dx11 case in
DXVAVieoDecodeAccelerator.
> > > 
> > > I feel pretty strongly that adding DXVA and DXVA2 accessors is the right
> thing
> > > to do -- it's really not much work to add those helper functions.
> > > 
> > > If you feel very very strongly that you don't want to do that, then I
guess
> > it's
> > > fine and I can lgtm it, but I still think it's the wrong approach.
> > 
> > It looks like MFT enums uses the same values as DXVA2, so if I write DXVA2
> > converters/accessors they
> > can be re-used for MFT (with a cast).
> 
> I think I'll have to use a helper class, because including the required
windows
> headers in color_space.h seems to create a bunch of naming conflicts in other
> parts
> of the code...

PTAL

[commit-bot: I haz the power, 2016-11-17 00:54:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbauman, 2016-11-17 01:02:22]

https://codereview.chromium.org/2503063002/diff/20001/media/gpu/dxva_video_de...
File media/gpu/dxva_video_decode_accelerator_win.cc (right):

https://codereview.chromium.org/2503063002/diff/20001/media/gpu/dxva_video_de...
media/gpu/dxva_video_decode_accelerator_win.cc:596: LOG(ERROR) << "DX9
DECODING";
Remove these.

https://codereview.chromium.org/2503063002/diff/20001/media/gpu/dxva_video_de...
media/gpu/dxva_video_decode_accelerator_win.cc:716: HRESULT hr =
DXVA2CreateVideoService(d3d9_device_ex_.get(),
Might be a good idea to add a finch trial for this in case we encounter crazy
driver bugs or performance issues.

[commit-bot: I haz the power, 2016-11-17 02:22:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 02:22:46]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[hubbe, 2016-11-17 18:46:03]

Description was changed from

==========
media: Use VideoProcessBlt on DX9

This should fix most range and color errors on windows 7.

BUG=655417, 650977, 576419, 576411
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
This should a bunch of color decoding bugs on windows.
For DX9, this means using VideoProcessBlt instead of StretchRect()
(There is a fallback path for StretchRect, not sure if that is needed.)
This is controlled by "--enable-features=video-blit-color-accuracy" which
defaults to off and will be enabled by an experiment.

BUG=655417, 650977, 576419, 576411
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[hubbe, 2016-11-17 18:46:17]

The CQ bit was checked by hubbe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 18:46:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hubbe, 2016-11-17 18:46:59]

Updated dx11 path.
Now flag-controlled.
Updated CL description.

PTAL.

https://codereview.chromium.org/2503063002/diff/20001/media/gpu/dxva_video_de...
File media/gpu/dxva_video_decode_accelerator_win.cc (right):

https://codereview.chromium.org/2503063002/diff/20001/media/gpu/dxva_video_de...
media/gpu/dxva_video_decode_accelerator_win.cc:596: LOG(ERROR) << "DX9
DECODING";
On 2016/11/17 01:02:22, jbauman wrote:
> Remove these.

Oops, done.

https://codereview.chromium.org/2503063002/diff/20001/media/gpu/dxva_video_de...
media/gpu/dxva_video_decode_accelerator_win.cc:716: HRESULT hr =
DXVA2CreateVideoService(d3d9_device_ex_.get(),
On 2016/11/17 01:02:22, jbauman wrote:
> Might be a good idea to add a finch trial for this in case we encounter crazy
> driver bugs or performance issues.

Done.

[commit-bot: I haz the power, 2016-11-17 19:46:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 19:47:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ccameron, 2016-11-17 19:54:44]

lgtm

[hubbe, 2016-11-22 18:10:36]

Ping?

[jbauman, 2016-11-22 22:16:59]

https://codereview.chromium.org/2503063002/diff/40001/media/gpu/dxva_video_de...
File media/gpu/dxva_video_decode_accelerator_win.cc (right):

https://codereview.chromium.org/2503063002/diff/40001/media/gpu/dxva_video_de...
media/gpu/dxva_video_decode_accelerator_win.cc:2691: return true;
We may need to reinitialize this if the color space changes.

[hubbe, 2016-11-23 21:21:01]

The CQ bit was checked by hubbe@chromium.org to run a CQ dry run

[hubbe, 2016-11-23 21:21:26]

https://codereview.chromium.org/2503063002/diff/40001/media/gpu/dxva_video_de...
File media/gpu/dxva_video_decode_accelerator_win.cc (right):

https://codereview.chromium.org/2503063002/diff/40001/media/gpu/dxva_video_de...
media/gpu/dxva_video_decode_accelerator_win.cc:2691: return true;
On 2016/11/22 22:16:59, jbauman wrote:
> We may need to reinitialize this if the color space changes.

Done.

[commit-bot: I haz the power, 2016-11-23 21:21:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbauman, 2016-11-23 22:20:14]

https://codereview.chromium.org/2503063002/diff/60001/media/gpu/dxva_video_de...
File media/gpu/dxva_video_decode_accelerator_win.cc (right):

https://codereview.chromium.org/2503063002/diff/60001/media/gpu/dxva_video_de...
media/gpu/dxva_video_decode_accelerator_win.cc:2718: video_format_converter_mft_
= NULL;
You can't set this to NULL as the check below will fail. I think it should be ok
to just reinitialize the MFT without setting it to null first.

[commit-bot: I haz the power, 2016-11-23 23:07:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 23:07:06]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[hubbe, 2016-11-23 23:58:37]

The CQ bit was checked by hubbe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 23:59:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hubbe, 2016-11-23 23:59:21]

https://codereview.chromium.org/2503063002/diff/60001/media/gpu/dxva_video_de...
File media/gpu/dxva_video_decode_accelerator_win.cc (right):

https://codereview.chromium.org/2503063002/diff/60001/media/gpu/dxva_video_de...
media/gpu/dxva_video_decode_accelerator_win.cc:2718: video_format_converter_mft_
= NULL;
On 2016/11/23 22:20:14, jbauman wrote:
> You can't set this to NULL as the check below will fail. I think it should be
ok
> to just reinitialize the MFT without setting it to null first.

Ops, I read that backwards.
Fixed.

[jbauman, 2016-11-24 00:01:49]

lgtm

[hubbe, 2016-11-24 00:26:23]

The CQ bit was checked by hubbe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 00:27:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 02:03:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 02:03:50]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[hubbe, 2016-11-29 18:30:13]

The CQ bit was checked by hubbe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 18:30:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 20:34:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 20:34:56]

Dry run: This issue passed the CQ dry run.

[hubbe, 2016-11-29 20:47:20]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2016-11-29 20:47:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org, jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2503063002/#ps120001
(title: "compile fix")

[commit-bot: I haz the power, 2016-11-29 20:48:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 20:59:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 20:59:25]

Failed to apply patch for media/base/media_switches.h:
While running git apply --index -p1;
  error: patch failed: media/base/media_switches.h:87
  error: media/base/media_switches.h: patch does not apply

Patch:       media/base/media_switches.h
Index: media/base/media_switches.h
diff --git a/media/base/media_switches.h b/media/base/media_switches.h
index
13f8bfe9f9770f2ba2df3e88f96783c871972681..7da1bd027ca608c08cd511f1adfae34aaec72e9e
100644
--- a/media/base/media_switches.h
+++ b/media/base/media_switches.h
@@ -87,6 +87,7 @@ MEDIA_EXPORT extern const base::Feature
kOverlayFullscreenVideo;
 MEDIA_EXPORT extern const base::Feature kResumeBackgroundVideo;
 MEDIA_EXPORT extern const base::Feature kUseNewMediaCache;
 MEDIA_EXPORT extern const base::Feature kVideoColorManagement;
+MEDIA_EXPORT extern const base::Feature kVideoBlitColorAccuracy;
 MEDIA_EXPORT extern const base::Feature kExternalClearKeyForTesting;
 
 #if defined(OS_ANDROID)

[hubbe, 2016-11-30 01:00:26]

The CQ bit was checked by hubbe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 01:01:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 03:05:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 03:05:37]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_optional_gpu_tests_rel on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[hubbe, 2016-11-30 03:21:32]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2016-11-30 03:21:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbauman@chromium.org, ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/2503063002/#ps140001
(title: "merged")

[commit-bot: I haz the power, 2016-11-30 03:22:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 05:24:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 05:24:07]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[hubbe, 2016-11-30 20:34:17]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2016-11-30 20:34:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 21:53:24]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480538057708690,
"parent_rev": "49071e4ca33b42825afbf792581302c15a3b9c2e", "commit_rev":
"a248cfc4f5c2c0b906bc9afe744b8df7c9791708"}

[commit-bot: I haz the power, 2016-11-30 21:53:54]

Description was changed from

==========
This should a bunch of color decoding bugs on windows.
For DX9, this means using VideoProcessBlt instead of StretchRect()
(There is a fallback path for StretchRect, not sure if that is needed.)
This is controlled by "--enable-features=video-blit-color-accuracy" which
defaults to off and will be enabled by an experiment.

BUG=655417, 650977, 576419, 576411
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
This should a bunch of color decoding bugs on windows.
For DX9, this means using VideoProcessBlt instead of StretchRect()
(There is a fallback path for StretchRect, not sure if that is needed.)
This is controlled by "--enable-features=video-blit-color-accuracy" which
defaults to off and will be enabled by an experiment.

BUG=655417, 650977, 576419, 576411
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-11-30 21:53:55]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-30 21:56:16]

Description was changed from

==========
This should a bunch of color decoding bugs on windows.
For DX9, this means using VideoProcessBlt instead of StretchRect()
(There is a fallback path for StretchRect, not sure if that is needed.)
This is controlled by "--enable-features=video-blit-color-accuracy" which
defaults to off and will be enabled by an experiment.

BUG=655417, 650977, 576419, 576411
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
This should a bunch of color decoding bugs on windows.
For DX9, this means using VideoProcessBlt instead of StretchRect()
(There is a fallback path for StretchRect, not sure if that is needed.)
This is controlled by "--enable-features=video-blit-color-accuracy" which
defaults to off and will be enabled by an experiment.

BUG=655417, 650977, 576419, 576411
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/070e970240f13d29be8930f9316f39ace711a116
Cr-Commit-Position: refs/heads/master@{#435431}
==========

[commit-bot: I haz the power, 2016-11-30 21:56:17]

Patchset 8 (id:??) landed as
https://crrev.com/070e970240f13d29be8930f9316f39ace711a116
Cr-Commit-Position: refs/heads/master@{#435431}

[brucedawson, 2016-12-06 21:41:14]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2016-12-06 21:41:15]

https://codereview.chromium.org/2503063002/diff/140001/media/gpu/dxva_video_d...
File media/gpu/dxva_video_decode_accelerator_win.cc (right):

https://codereview.chromium.org/2503063002/diff/140001/media/gpu/dxva_video_d...
media/gpu/dxva_video_decode_accelerator_win.cc:754: if (hr)
The experimental VC++ /analyze builder gives the following on this line and the
other three checks of |hr|:

warning C6230: Implicit cast between semantically different integer types: 
using HRESULT in a Boolean context.

It wants you to use if (FAILED(hr)) instead. The difference is that the FAILED
macro checks whether hr is negative or not - a positive but non-zero result does
not actually indicate a failure.

If the non-standard use of hr is intended then a comment is probably worthwhile.
If not intended then if (FAILED(hr)) would be better I think.

[hubbe, 2016-12-06 21:46:34]

https://codereview.chromium.org/2503063002/diff/140001/media/gpu/dxva_video_d...
File media/gpu/dxva_video_decode_accelerator_win.cc (right):

https://codereview.chromium.org/2503063002/diff/140001/media/gpu/dxva_video_d...
media/gpu/dxva_video_decode_accelerator_win.cc:754: if (hr)
On 2016/12/06 21:41:15, brucedawson wrote:
> The experimental VC++ /analyze builder gives the following on this line and
the
> other three checks of |hr|:
> 
> warning C6230: Implicit cast between semantically different integer types: 
> using HRESULT in a Boolean context.
> 
> It wants you to use if (FAILED(hr)) instead. The difference is that the FAILED
> macro checks whether hr is negative or not - a positive but non-zero result
does
> not actually indicate a failure.
> 
> If the non-standard use of hr is intended then a comment is probably
worthwhile.
> If not intended then if (FAILED(hr)) would be better I think.

Will fix in a followup CL.