Use InterlockedAdd(0) to get the current state in DXVAVideoDecodeAccelerator::GetState

Restore DXVAVideoDecodeAccelerator::SetState to continue setting the state on the main thread.

DXVAVideoDecodeAccelerator::GetState uses InterlockedAdd(0) to get the recent state.

BUG=441000
Committed: https://crrev.com/fc1bdb4f2b45cb771e227238241646ac18d249ca
Cr-Commit-Position: refs/heads/master@{#308245}

[ananta, 2014-12-11 01:09:57]

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

[DaleCurtis, 2014-12-11 20:52:12]

This is only marginally better than what you have now; all code using the state
has to be under lock too to ensure that the state doesn't change in the middle
of processing.

The simplest way to do that is avoid any state queries on the decoder thread and
instead use posted tasks and weakptrs to cancel tasks from the main thread when
the state changes.

lgtm to remove the interlocked usage, but note this doesn't actually make the
threading story any more coherent.

[ananta, 2014-12-11 22:10:01]

Patchset #3 (id:40001) has been deleted

[brucedawson, 2014-12-12 00:31:21]

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

[brucedawson, 2014-12-12 00:31:22]

I'm commenting on the latest revisions, which use InterlockedAdd(0) and
InterlockedExchange, whereas DaleCurtis@ commented on an intermediate version
that had short-scoped locks. The new code ensures an atomic read of state_,
which is progress.

I agree with DaleCurtis@ that avoiding state queries on the decoder thread is
the long-term way to go. It should be clear what lock is protecting all shared
variables, and groups of related operations should be done without releasing the
lock in the middle or else coherence can be lost.

I found the suggestions in the article linked below quite relevant. It
recommends particular design patterns and commenting conventions which could be
very helpful for future changes:

http://www.chromium.org/developers/lock-and-condition-variable

but, with the caveats above:
lgtm

[brucedawson, 2014-12-12 18:56:29]

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

https://codereview.chromium.org/793993002/diff/60001/content/common/gpu/media...
content/common/gpu/media/dxva_video_decode_accelerator.cc:1462:
InterlockedAdd(reinterpret_cast<volatile long*>(&state_), 0));
When doing a reinterpret_cast between State* and long* it is worthwhile to
static_assert that the types being pointed at are indeed the same size.

I know that they are the same size in this case, but a static assert would
document this assumption and ensure that it stays true.

[ananta, 2014-12-12 19:59:07]

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

https://codereview.chromium.org/793993002/diff/60001/content/common/gpu/media...
content/common/gpu/media/dxva_video_decode_accelerator.cc:1462:
InterlockedAdd(reinterpret_cast<volatile long*>(&state_), 0));
On 2014/12/12 18:56:29, brucedawson wrote:
> When doing a reinterpret_cast between State* and long* it is worthwhile to
> static_assert that the types being pointed at are indeed the same size.
> 
> I know that they are the same size in this case, but a static assert would
> document this assumption and ensure that it stays true.

Done.

[brucedawson, 2014-12-12 20:50:18]

https://codereview.chromium.org/793993002/diff/80001/content/common/gpu/media...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/793993002/diff/80001/content/common/gpu/media...
content/common/gpu/media/dxva_video_decode_accelerator.cc:1461:
DCHECK(sizeof(State) == sizeof(long));
You shouldn't use DCHECK here -- use static_assert instead, as in:

static_assert(sizeof(State) == sizeof(long), "mismatched type sizes");

This ensures that any failures are caught at compile-time, for debug and release
builds. It also ensures that there is zero overhead.

[ananta, 2014-12-12 20:58:59]

https://codereview.chromium.org/793993002/diff/80001/content/common/gpu/media...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/793993002/diff/80001/content/common/gpu/media...
content/common/gpu/media/dxva_video_decode_accelerator.cc:1461:
DCHECK(sizeof(State) == sizeof(long));
On 2014/12/12 20:50:18, brucedawson wrote:
> You shouldn't use DCHECK here -- use static_assert instead, as in:
> 
> static_assert(sizeof(State) == sizeof(long), "mismatched type sizes");
> 
> This ensures that any failures are caught at compile-time, for debug and
release
> builds. It also ensures that there is zero overhead.

Thanks. done.

[brucedawson, 2014-12-12 21:01:33]

lgtm

[ananta, 2014-12-13 00:11:21]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-12-13 00:13:18]

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

[commit-bot: I haz the power, 2014-12-13 02:59:02]

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

[commit-bot: I haz the power, 2014-12-13 02:59:03]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ananta, 2014-12-13 02:59:33]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-12-13 03:00:38]

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

[commit-bot: I haz the power, 2014-12-13 03:52:55]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2014-12-13 03:53:39]

Patchset 5 (id:??) landed as
https://crrev.com/fc1bdb4f2b45cb771e227238241646ac18d249ca
Cr-Commit-Position: refs/heads/master@{#308245}