Use NotifyError() to report errors in VTVideoDecodeAccelerator.

Use NotifyError() to report errors in VTVideoDecodeAccelerator.

BUG=133828
Committed: https://crrev.com/b43e0d0bc14a6554819d54523072028396124687
Cr-Commit-Position: refs/heads/master@{#302182}

[sandersd (OOO until July 31), 2014-10-09 00:27:25]

sandersd@chromium.org changed reviewers:
+ posciak@chromium.org

[Pawel Osciak, 2014-10-22 10:45:21]

https://chromiumcodereview.appspot.com/642453003/diff/60001/content/common/gp...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/642453003/diff/60001/content/common/gp...
content/common/gpu/media/vt_video_decode_accelerator.cc:24: #define
NOTIFY_STATUS(name, status)                            \
I'd personally prefer s/STATUS/ERROR/, but up to you.

https://chromiumcodereview.appspot.com/642453003/diff/60001/content/common/gp...
content/common/gpu/media/vt_video_decode_accelerator.cc:487: case ACTION_RESET:
Do we still want to handle this here if has_error_?

https://chromiumcodereview.appspot.com/642453003/diff/60001/content/common/gp...
content/common/gpu/media/vt_video_decode_accelerator.cc:627: void
VTVideoDecodeAccelerator::DropFrame(int32_t bitstream_id) {
There's a lot of DropFrame() around and it makes me worried it could be easily
forgotten somewhere at some point in the future even.
Could we instead wrap BitstreamBuffer in a class that would DropFrame() in
destructor?

https://chromiumcodereview.appspot.com/642453003/diff/60001/content/common/gp...
content/common/gpu/media/vt_video_decode_accelerator.cc:629:
gpu_task_runner_->PostTask(FROM_HERE, base::Bind(
It's not exactly clear to me why we need to dutifully DropFrames, instead of
just destroying ourselves and returning everything from there? Could you explain
this somewhere please?

[sandersd (OOO until July 31), 2014-10-22 22:15:45]

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

https://codereview.chromium.org/642453003/diff/60001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.cc:24: #define
NOTIFY_STATUS(name, status)                            \
On 2014/10/22 10:45:21, Pawel Osciak wrote:
> I'd personally prefer s/STATUS/ERROR/, but up to you.

While this is a wrapper for NotifyError(), it is specific to notifying about
statuses; I'd prefer to leave the distinction.

https://codereview.chromium.org/642453003/diff/60001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.cc:487: case ACTION_RESET:
On 2014/10/22 10:45:21, Pawel Osciak wrote:
> Do we still want to handle this here if has_error_?

I couldn't tell what the correct behavior was here; GVD does ensure that all
resets are completed as part of destruction, so for Chromium it shouldn't
matter.

I've gone ahead and prevented this case as well.

https://codereview.chromium.org/642453003/diff/60001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.cc:627: void
VTVideoDecodeAccelerator::DropFrame(int32_t bitstream_id) {
On 2014/10/22 10:45:21, Pawel Osciak wrote:
> There's a lot of DropFrame() around and it makes me worried it could be easily
> forgotten somewhere at some point in the future even.
> Could we instead wrap BitstreamBuffer in a class that would DropFrame() in
> destructor?

I don't really see an alternative; either we successfully pass it to
VTDecompressionSessionDecodeFrame() or we have to drop it. There is a similar
situation once the decoded frame is back in the main thread (except using
NotifyEndOfBitstreamBuffer() instead of DropFrame()).

I've implemented an RAII approach that eliminates all but the two in
OutputTask().

https://codereview.chromium.org/642453003/diff/60001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.cc:629:
gpu_task_runner_->PostTask(FROM_HERE, base::Bind(
On 2014/10/22 10:45:21, Pawel Osciak wrote:
> It's not exactly clear to me why we need to dutifully DropFrames, instead of
> just destroying ourselves and returning everything from there? Could you
explain
> this somewhere please?

There is a comment now where the ScopedClosureRunner is created. The deeper
explanation is that we must ack every frame or the GPU thread will get confused
about what frames are currently being decoded.

[Pawel Osciak, 2014-10-30 10:35:13]

lgtm % nits

https://chromiumcodereview.appspot.com/642453003/diff/140001/content/common/g...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/642453003/diff/140001/content/common/g...
content/common/gpu/media/vt_video_decode_accelerator.cc:374: // Now that the
bitstream is decoding, don't drop it.
At this point decoding is not done yet? It is done I'd thought?
(I know you want to not return it so that's why release, but the comment is
misleading?)

https://chromiumcodereview.appspot.com/642453003/diff/140001/content/common/g...
content/common/gpu/media/vt_video_decode_accelerator.cc:375:
(void)drop_bitstream.Release();
Do we need this (void) ?

https://chromiumcodereview.appspot.com/642453003/diff/140001/content/common/g...
content/common/gpu/media/vt_video_decode_accelerator.cc:387: } else  if
(CFGetTypeID(image_buffer) != CVPixelBufferGetTypeID()) {
s/else  if/else if/

[sandersd (OOO until July 31), 2014-10-30 22:17:03]

https://codereview.chromium.org/642453003/diff/140001/content/common/gpu/medi...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/642453003/diff/140001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:374: // Now that the
bitstream is decoding, don't drop it.
On 2014/10/30 10:35:13, Pawel Osciak wrote:
> At this point decoding is not done yet? It is done I'd thought?
> (I know you want to not return it so that's why release, but the comment is
> misleading?)

The comment is correct, the frame has been submitted async to VT.

If we switched to sync it would fix the dropped frame reordering issue, but
that's not in this CL.

https://codereview.chromium.org/642453003/diff/140001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:375:
(void)drop_bitstream.Release();
On 2014/10/30 10:35:13, Pawel Osciak wrote:
> Do we need this (void) ?

Release() is WARN_UNUSED_RESULT.

https://codereview.chromium.org/642453003/diff/140001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:387: } else  if
(CFGetTypeID(image_buffer) != CVPixelBufferGetTypeID()) {
On 2014/10/30 10:35:13, Pawel Osciak wrote:
> s/else  if/else if/

Done.

[sandersd (OOO until July 31), 2014-10-30 22:17:41]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-10-30 22:18:55]

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

[commit-bot: I haz the power, 2014-10-30 23:02:50]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-10-30 23:03:38]

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