Support H.264 video decoding on Windows 8+ using DX11

Add support for DX11 based H/W video decoding on Windows 8+

    This is only available on Windows 8+ because the media foundation API which exposes the device
    manager to be pased to the decoder MFCreateDXGIDeviceManager only exists on Windows 8+.

    Changes in this patch are mostly around using DX11 or D3D wherever needed. These are as below:-

    1. In the initialization code path where we use D3D or DX11 based on whether we are on Windows 8+
       and ANGLE and the decoder say they support DX11.

    2. The output frame processing code where we extract the DX11 texture and copy it out to ANGLE. One change
       here is that DX11 does not provide an automatic way for format conversion for textures. The decoder outputs
       YUV12 textures and ANGLE expects RGB. We can achieve this by setting a shader for conversion. That seemed
       like too much work. Thankfully there is a video processor media foundation transform on Windows which
       does the conversion for us on the GPU. We use this object to convert the output frame and copy it out to
       ANGLE.

    3. We pass the GL context to the decoder to enable us to query ANGLE to see if it is using D3D or DX11.

    BUG=456418
Committed: https://crrev.com/3b01db9b22725d4e7d6422d8dadf48dce8ac4794
Cr-Commit-Position: refs/heads/master@{#318560}

[ananta, 2015-02-25 02:57:46]

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

[ananta, 2015-02-25 02:59:31]

ananta@chromium.org changed reviewers:
+ cpu@chromium.org

[DaleCurtis, 2015-02-25 03:05:52]

Hmm, I'm surprised we can only use this on Windows 8+. What performs better
after this change?

[ananta, 2015-02-25 03:19:52]

On 2015/02/25 03:05:52, DaleCurtis wrote:
> Hmm, I'm surprised we can only use this on Windows 8+. What performs better
> after this change?

On Hybrid systems with integrated and discrete GPU's there are bugs in drivers
which are seen when D3D textures and DX11 textures
occur on different GPU's. This causes the videos to show up black. 

There was a change a while back to blacklist h/w video decode on these machines.
There was a thread recently with AMD engineers
which confirms this problem.

The theory is that if both the H/W video decoder and ANGLE use DX11 textures
then this problem goes away.

We are also hoping that there will be a performance improvement in video
playback with this change.

[cpu_(ooo_6.6-7.5), 2015-02-25 23:03:36]

The issue was explained to me. ATI folks reached to the GPU folks saying that
some driver + GPU combos hit slow paths (or hangs, I don't recall) when you mix
DX11 and DX9Ex textures.

The inter-op is mentioned in msdn, so we are not doing something bad, is just
that is probably a corner case from the driver perspective.

The outer compositor is in most/many machines DX11 and the video texture was
always DX9Ex.  Also when Ananta landed the code initially we did not had DX11.

[cpu_(ooo_6.6-7.5), 2015-02-25 23:04:57]

please change the patch description D3D --> DX9

[cpu_(ooo_6.6-7.5), 2015-02-25 23:44:30]

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:354: // TODO(dshwang):
after moving to D3D11, use RGBA surface. crbug.com/438691
aren't we moving to DX11?  can you check that bug?

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:555: // On Windows 7
mshtmlmedia.dll provides it.
I am unclear how this device manager is used, the documentation says "Enables
two threads to share the same Microsoft Direct3D 11 device." but I don't see
calls to LockDevice()... how is it used?

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:1771:
src_texture->GetDesc(&source_desc);
check for error in GetDesc?

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.h (right):

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.h:40:
IMFDXGIDeviceManager** device_manager);
note that you can now use alias

http://en.cppreference.com/w/cpp/language/type_alias

in this case it would look nicer, if you include the right header:

alias CreateDXGIManager = MFCreateDXGIDeviceManager;

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.h:249:
base::win::ScopedComPtr<IDirect3DDevice9Ex> device_;
I think we should rename device_ to d3d9_device_

[rvargas (doing something else), 2015-02-26 02:09:55]

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

[rvargas (doing something else), 2015-02-26 02:09:56]

random drive-by.

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.h (right):

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.h:40:
IMFDXGIDeviceManager** device_manager);
On 2015/02/25 23:44:30, cpu wrote:
> note that you can now use alias
> 
> http://en.cppreference.com/w/cpp/language/type_alias
> 
> in this case it would look nicer, if you include the right header:
> 
> alias CreateDXGIManager = MFCreateDXGIDeviceManager;
>  

I'm a bit confused by this. MFCreateDXGIDeviceManager is not the type of a
function pointer, but a function declaration, right? If so, the alias would have
to be to a weird function pointer declaration, no?

If that is the case, I don't think we should use an alias (function pointer
declarations are weird enough as they are, to drop another syntax on top). If
there is a typedef somewhere that allows something like what you wrote then sure
:).

[ananta, 2015-02-26 02:18:34]

Sadly the hack to get DX11 decoding to work on Windows 7 does not work. We can
get a pointer to the MFCreateDXGIDeviceManager function and the device creation
works. However the IMFTransform we use for color space conversion
(https://msdn.microsoft.com/en-us/library/windows/desktop/hh162913%28v=vs.85%2...)
does not exist on Windows 7. There is a color converter DMO which technically
could work. However it needs the video samples to expose functioning IMF2DBuffer
interfaces which is not the case sadly.

I left the code to get the pointer to the MFCreateDXGIDeviceManager function
under an ifdef for Windows 7. I will look into this at a later point in time.

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:354: // TODO(dshwang):
after moving to D3D11, use RGBA surface. crbug.com/438691
On 2015/02/25 23:44:30, cpu wrote:
> aren't we moving to DX11?  can you check that bug?

Removed the TODO. We aren't moving exactly. We will still support dx9 if needed.

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:555: // On Windows 7
mshtmlmedia.dll provides it.
On 2015/02/25 23:44:30, cpu wrote:
> I am unclear how this device manager is used, the documentation says "Enables
> two threads to share the same Microsoft Direct3D 11 device." but I don't see
> calls to LockDevice()... how is it used?

The transform internally calls LockDevice if needed.

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.h (right):

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.h:40:
IMFDXGIDeviceManager** device_manager);
On 2015/02/25 23:44:30, cpu wrote:
> note that you can now use alias
> 
> http://en.cppreference.com/w/cpp/language/type_alias
> 
> in this case it would look nicer, if you include the right header:
> 
> alias CreateDXGIManager = MFCreateDXGIDeviceManager;
>  

I dunno what you are indicating here. We can get rid of typedef and replace this
with something on the lines of 

using func = HRESULT (*) (uint*,IMFDXGIDeviceManger**);

I am not sure if that is more readable. Leaving this for now

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.h:249:
base::win::ScopedComPtr<IDirect3DDevice9Ex> device_;
On 2015/02/25 23:44:30, cpu wrote:
> I think we should rename device_ to d3d9_device_

Done. Renamed it to d3d9_device_ex_

[DaleCurtis, 2015-02-26 22:32:22]

I defer to cpu, rvargas for a lot of the Windows specific bits, but overall this
looks fine to me.

https://codereview.chromium.org/922003002/diff/200001/content/common/BUILD.gn
File content/common/BUILD.gn (right):

https://codereview.chromium.org/922003002/diff/200001/content/common/BUILD.gn...
content/common/BUILD.gn:413: "d3d11.lib",
Needs the ldflags update you made to the .gypi below?

https://codereview.chromium.org/922003002/diff/200001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/922003002/diff/200001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:703: // TODO(ananta)
Do you want to alias GetLastError() on the stack ahead of time to debug these?

https://codereview.chromium.org/922003002/diff/200001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:1052: hr =
attributes->GetUINT32(MF_SA_D3D11_AWARE, &dx11_aware);
Remove hr= if you're not going to use it?

https://codereview.chromium.org/922003002/diff/200001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:1796: CHECK(false);
Ditto for GetLastError alias.

https://codereview.chromium.org/922003002/diff/200001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:1839: // video
processor coverts the color space of the output frame and copies
Neat, is this more efficient than our existing copy?

https://codereview.chromium.org/922003002/diff/200001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:1866: hr =
video_format_converter_mft_->ProcessOutput(
Does this complete immediately? Unlike StretchRect...

[ananta, 2015-02-26 23:32:57]

https://codereview.chromium.org/922003002/diff/200001/content/common/BUILD.gn
File content/common/BUILD.gn (right):

https://codereview.chromium.org/922003002/diff/200001/content/common/BUILD.gn...
content/common/BUILD.gn:413: "d3d11.lib",
On 2015/02/26 22:32:22, DaleCurtis wrote:
> Needs the ldflags update you made to the .gypi below?

Yes. Thanks for pointing that out.

https://codereview.chromium.org/922003002/diff/200001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/922003002/diff/200001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:703: // TODO(ananta)
On 2015/02/26 22:32:22, DaleCurtis wrote:
> Do you want to alias GetLastError() on the stack ahead of time to debug these?

Yes. Done.

https://codereview.chromium.org/922003002/diff/200001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:1052: hr =
attributes->GetUINT32(MF_SA_D3D11_AWARE, &dx11_aware);
On 2015/02/26 22:32:22, DaleCurtis wrote:
> Remove hr= if you're not going to use it?

Done.

https://codereview.chromium.org/922003002/diff/200001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:1796: CHECK(false);
On 2015/02/26 22:32:22, DaleCurtis wrote:
> Ditto for GetLastError alias.

Added the alias to the InitializeDX11VideoFormatConverterMediaType and removed
the check here.

https://codereview.chromium.org/922003002/diff/200001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:1839: // video
processor coverts the color space of the output frame and copies
On 2015/02/26 22:32:22, DaleCurtis wrote:
> Neat, is this more efficient than our existing copy?

The existing copy in D3D9 occurs in the GPU process. It is still a copy across
devices with any locking etc being performed by the API.

The video processor route in DX11 occurs in the GPU process and does any locking
etc if needed.

On a related note this will all go away once ANGLE folks land the extension to
allow a caller to retrieve the ANGLE device object. We can avoid creating a
device in the decoder then.

https://codereview.chromium.org/922003002/diff/200001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:1866: hr =
video_format_converter_mft_->ProcessOutput(
On 2015/02/26 22:32:22, DaleCurtis wrote:
> Does this complete immediately? Unlike StretchRect...

Yes. This transform internally retrieves the bits from the videoframe via the
Lock operation on the frames buffer and then converts the frame from NV12 to
whatever we want. When ProcessOutput returns we have the converted bits. We thus
don't need to call Flush and wait for the command set to complete.

[cpu_(ooo_6.6-7.5), 2015-02-26 23:42:09]

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.h (right):

https://codereview.chromium.org/922003002/diff/160001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.h:40:
IMFDXGIDeviceManager** device_manager);
On 2015/02/26 02:18:34, ananta wrote:
> On 2015/02/25 23:44:30, cpu wrote:
> > note that you can now use alias
> > 
> > http://en.cppreference.com/w/cpp/language/type_alias
> > 
> > in this case it would look nicer, if you include the right header:
> > 
> > alias CreateDXGIManager = MFCreateDXGIDeviceManager;
> >  
> 
> I dunno what you are indicating here. We can get rid of typedef and replace
this
> with something on the lines of 
> 
> using func = HRESULT (*) (uint*,IMFDXGIDeviceManger**);
> 
> I am not sure if that is more readable. Leaving this for now

Yeah, it lacks more magic. It is fine as is until more magic arrives.

[cpu_(ooo_6.6-7.5), 2015-02-27 00:02:06]

lgtm

please have an owner on content\gpu , possibly john bauman give a quick review.

[DaleCurtis, 2015-02-27 00:42:55]

https://codereview.chromium.org/922003002/diff/220001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/922003002/diff/220001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:703:
base::debug::Alias(&hr);
Why not under FAILED()? Also will hr be synonymous with GetLastError() in this
case? I suspect you want both. Ditto for the other CHECK(false) pieces.

[ananta, 2015-02-27 01:49:07]

https://codereview.chromium.org/922003002/diff/220001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/922003002/diff/220001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:703:
base::debug::Alias(&hr);
On 2015/02/27 00:42:55, DaleCurtis wrote:
> Why not under FAILED()? Also will hr be synonymous with GetLastError() in this
> case? I suspect you want both. Ditto for the other CHECK(false) pieces.

hr will give us all information we need, i.e a media foundation error code. We
don't need GetLastError here. I moved the alias call to under failed hr here and
the other places.

[DaleCurtis, 2015-02-27 01:55:03]

lgtm

[ananta, 2015-02-27 01:55:38]

ananta@chromium.org changed reviewers:
+ jbauman@chromium.org

[jbauman, 2015-02-27 02:08:34]

https://codereview.chromium.org/922003002/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/922003002/diff/260001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:1817:
input_buffer_id));
I think you need something like DXVAVideoDecodeAccelerator::FlushDecoder with
D3D11_QUERY_EVENT to ensure that the the video decoder context finishes writing
this before ANGLE reads it.

[ananta, 2015-02-27 23:18:33]

https://codereview.chromium.org/922003002/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/922003002/diff/260001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:1817:
input_buffer_id));
On 2015/02/27 02:08:34, jbauman wrote:
> I think you need something like DXVAVideoDecodeAccelerator::FlushDecoder with
> D3D11_QUERY_EVENT to ensure that the the video decoder context finishes
writing
> this before ANGLE reads it.

Done.

[jbauman, 2015-02-27 23:23:18]

lgtm. Thanks for figuring all of this out.

[ananta, 2015-02-27 23:46:39]

The CQ bit was checked by ananta@chromium.org

[ananta, 2015-02-27 23:46:39]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/922003002/#ps300001
(title: "Added a CHECK to catch GetData failures")

[commit-bot: I haz the power, 2015-02-27 23:47:46]

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

[commit-bot: I haz the power, 2015-02-28 00:39:28]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-02-28 00:40:14]

Patchset 16 (id:??) landed as
https://crrev.com/3b01db9b22725d4e7d6422d8dadf48dce8ac4794
Cr-Commit-Position: refs/heads/master@{#318560}