Lock the decoder device (Angle) device from the decoder thread before copying the decoded frame to …

Lock the decoder device (Angle) device from the decoder thread before copying the decoded frame to the target texture.

It appears that the change to use the angle device in the dxva decoder has caused the webgl conformance tests to fail
intermittently. The reason that this may be happening is because the device is accessed from multiple threads. The critical
code path here seems to be the copying of the decoded texture to the target texture created by angle.

Proposed fix is to lock the device prior to the copy. If this works, then we can look at alternative approaches like moving
the copy back to the main thread, or do away with the copy altogether.

BUG=548383
R=dalecurtis

Committed: https://crrev.com/378834484abf86953f242dc6c9e6135ef2424e7c
Cr-Commit-Position: refs/heads/master@{#357487}

[ananta, 2015-10-31 03:05:48]

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

[ananta, 2015-10-31 07:13:43]

Please hold off on the review. Will update the patch.

[ananta, 2015-11-02 19:48:46]

PTAL

[DaleCurtis, 2015-11-02 19:58:30]

defer to jbauman, holding the "lock" across multiple threads and calls which
each have early exit paths seems sketchy though.

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

https://codereview.chromium.org/1427213002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:624:
weak_this_factory_(this),
Weak factory should always be last.

[ananta, 2015-11-02 20:01:37]

On 2015/11/02 19:58:30, DaleCurtis wrote:
> defer to jbauman, holding the "lock" across multiple threads and calls which
> each have early exit paths seems sketchy though.
> 
>
https://codereview.chromium.org/1427213002/diff/20001/content/common/gpu/medi...
> File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/1427213002/diff/20001/content/common/gpu/medi...
> content/common/gpu/media/dxva_video_decode_accelerator.cc:624:
> weak_this_factory_(this),
> Weak factory should always be last.

The lock is only grabbed by the decoder thread.

[jbauman, 2015-11-02 20:40:36]

On 2015/11/02 20:01:37, ananta wrote:
> On 2015/11/02 19:58:30, DaleCurtis wrote:
> > defer to jbauman, holding the "lock" across multiple threads and calls which
> > each have early exit paths seems sketchy though.
> > 
> >
>
https://codereview.chromium.org/1427213002/diff/20001/content/common/gpu/medi...
> > File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):
> > 
> >
>
https://codereview.chromium.org/1427213002/diff/20001/content/common/gpu/medi...
> > content/common/gpu/media/dxva_video_decode_accelerator.cc:624:
> > weak_this_factory_(this),
> > Weak factory should always be last.
> 
> The lock is only grabbed by the decoder thread.

I don't think there's much of a point in holding the lock only on the decoder
thread. If we want to prevent access on the main thread, then we need to grab
the lock there as well. Also, see
https://msdn.microsoft.com/en-us/library/windows/desktop/ff476891(v=vs.85).aspx
for more info on how D3D11 multithreading is supposed to work.

[ananta, 2015-11-02 20:48:17]

On 2015/11/02 20:40:36, jbauman wrote:
> On 2015/11/02 20:01:37, ananta wrote:
> > On 2015/11/02 19:58:30, DaleCurtis wrote:
> > > defer to jbauman, holding the "lock" across multiple threads and calls
which
> > > each have early exit paths seems sketchy though.
> > > 
> > >
> >
>
https://codereview.chromium.org/1427213002/diff/20001/content/common/gpu/medi...
> > > File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1427213002/diff/20001/content/common/gpu/medi...
> > > content/common/gpu/media/dxva_video_decode_accelerator.cc:624:
> > > weak_this_factory_(this),
> > > Weak factory should always be last.
> > 
> > The lock is only grabbed by the decoder thread.
> 
> I don't think there's much of a point in holding the lock only on the decoder
> thread. If we want to prevent access on the main thread, then we need to grab
> the lock there as well. Also, see
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ff476891(v=vs.85).aspx
> for more info on how D3D11 multithreading is supposed to work.

As per docs on the ID3D10Multithread::Enter method grabbing the lock on any
thread prevents other threads
from calling methods on the device if multi thread protection is enabled. This
was the behavior I was seeing
if I grabbed the lock and did not release it.

[jbauman, 2015-11-02 20:50:43]

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

https://codereview.chromium.org/1427213002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:2042:
decoder_thread_task_runner_->PostDelayedTask(
I think you should do the Leave here - it should be fine for other code to use
the D3D11 context while we're waiting for results.

https://codereview.chromium.org/1427213002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:2078: hr =
d3d11_device_context_->GetData(d3d11_query_.get(), &query_data,
You could maybe do an Enter() and Leave() just around this call.

[ananta, 2015-11-02 22:17:56]

Removed the wait for the flush to complete for dx11 and disable-d3d11 as we are
sharing the angle device in these cases.

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

https://codereview.chromium.org/1427213002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:624:
weak_this_factory_(this),
On 2015/11/02 19:58:30, DaleCurtis wrote:
> Weak factory should always be last.

Done.

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

https://codereview.chromium.org/1427213002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:2042:
decoder_thread_task_runner_->PostDelayedTask(
On 2015/11/02 20:50:42, jbauman wrote:
> I think you should do the Leave here - it should be fine for other code to use
> the D3D11 context while we're waiting for results.

We don't need to wait anymore for the flush based on our discussion about
sharing the angle device.

https://codereview.chromium.org/1427213002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:2078: hr =
d3d11_device_context_->GetData(d3d11_query_.get(), &query_data,
On 2015/11/02 20:50:42, jbauman wrote:
> You could maybe do an Enter() and Leave() just around this call.

Ditto

[jbauman, 2015-11-02 22:55:43]

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

https://codereview.chromium.org/1427213002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:729: dx11_disabled_ =
base::CommandLine::ForCurrentProcess()->HasSwitch(
I'm not sure why we're checking this command line flag. It's possible that
Chrome's using D3D9 even if the command-line isn't present, for example if the
GPU doesn't support D3D11. We should probably always query the D3D9 device from
ANGLE, and if we get it use it.

[ananta, 2015-11-02 23:11:54]

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

https://codereview.chromium.org/1427213002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:729: dx11_disabled_ =
base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/11/02 22:55:43, jbauman wrote:
> I'm not sure why we're checking this command line flag. It's possible that
> Chrome's using D3D9 even if the command-line isn't present, for example if the
> GPU doesn't support D3D11. We should probably always query the D3D9 device
from
> ANGLE, and if we get it use it.

Done.

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

lgtm

[DaleCurtis, 2015-11-03 00:03:09]

lgtm

[ananta, 2015-11-03 00:25:48]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-11-03 00:29:00]

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

[commit-bot: I haz the power, 2015-11-03 00:40:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-03 00:41:05]

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