Add accelerated video decoder interface, VP8 and H.264 implementations and hook up to V4L2SVDA.

Add accelerated video decoder interface, VP8 and H.264 implementations and hook up to V4L2SVDA.

An AcceleratedVideoDecoder is a video decoder that requires support from an
external accelerator (typically a hardware accelerator) to partially
offload the decode process after parsing stream headers, and performing
reference frame and state management.

In this design, the hardware-independent decoder implementation interfaces
with a HW-specific Accelerator to offload last stages of the decode process.

Add the interface for AcceleratedVideoDecoder, decoders for VP8 and H264,
V4L2-specific Accelerators for VP8 and H264 and a common accelerator/client
class for V4L2.

TEST=vdatest VP8/H264, mp4 local video playback, apprtc decode
BUG=chrome-os-partner:33728
Committed: https://crrev.com/04272d3017214b966feba379d78b195e2fd2193d
Cr-Commit-Position: refs/heads/master@{#311448}

[Pawel Osciak, 2015-01-08 09:19:37]

posciak@chromium.org changed reviewers:
+ kcwu@chromium.org, owenlin@chromium.org, wuchengli@chromium.org

[Owen Lin, 2015-01-08 15:42:06]

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/acc...
File content/common/gpu/media/accelerated_video_decoder.h (right):

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/acc...
content/common/gpu/media/accelerated_video_decoder.h:10: #include
"base/memory/ref_counted.h"
not used for the above includes?

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/acc...
content/common/gpu/media/accelerated_video_decoder.h:38: enum DecResult {
Use a full name, e.g., DecodeResult ?

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/acc...
content/common/gpu/media/accelerated_video_decoder.h:52: virtual DecResult
Decode() WARN_UNUSED_RESULT = 0;
Is this a blocking call? 
How many frame will be decoded? If not one, what happens if user want to seek or
stop the video?

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/acc...
content/common/gpu/media/accelerated_video_decoder.h:52: virtual DecResult
Decode() WARN_UNUSED_RESULT = 0;
How is the decoded frames be returned?

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/acc...
content/common/gpu/media/accelerated_video_decoder.h:57: virtual gfx::Size
GetPicSize() = 0;
return "const gfx::Size&" ?

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/acc...
content/common/gpu/media/accelerated_video_decoder.h:60:
DISALLOW_COPY_AND_ASSIGN(AcceleratedVideoDecoder);
You have put this under private: to make it works.

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/h26...
File content/common/gpu/media/h264_decoder.cc (right):

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_decoder.cc:306: for (auto& pic : dpb_) {
So, the type of pic is actually a "Picture*". In this case, there is no need to
make a reference to a pointer.

However, I would suggest explicit show the type here: 

for (H264Picture* pic : dpb_)

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/h26...
File content/common/gpu/media/h264_decoder.h (right):

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_decoder.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
2015? 
Remove (c)?

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_decoder.h:11: #include "base/memory/linked_ptr.h"
Are the above two includes not used?

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_decoder.h:55: virtual bool SubmitDecode(const
scoped_refptr<H264Picture>& pic) = 0;
Add docs. I cannot figure out how these function work, it seems not trivial.

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_decoder.h:58:
DISALLOW_COPY_AND_ASSIGN(H264Accelerator);
put it under "private:"

[Pawel Osciak, 2015-01-09 08:16:08]

Patchset #3 (id:40001) has been deleted

[Owen Lin, 2015-01-09 09:56:14]

I am still trying to understand the CL. So, there are some rough thoughts. We
may address/discuss them after the branch.

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
File content/common/gpu/media/accelerated_video_decoder.h (right):

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/accelerated_video_decoder.h:20: class CONTENT_EXPORT
AcceleratedVideoDecoder {
I didn't see how the decoded frames be returned in this class. After reading the
other parts, I realize the frame ares actually output by different Accelerators.
This sounds wired. I think it would be nice if we can have an interface here. By
doing so, the client of this class don't need to deal with different so called
Accelerators.

My proposal is adding:

  bool GetDecodedPicture(Picture* picture);

to this class, and change the OutputPicture of those Accelerators to actually to
export from the internal format to media::Picture.

e.g., bool Vp8Accelerator::OutputPicture(Vp8Picture in, media::Picture* out);

But this is just a rough thought.

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/accelerated_video_decoder.h:57: virtual gfx::Size
GetPicSize() = 0;
Would it be better to move into those Accelerator?

class Vp8Accelerator {
  int SetRequiredNumOfPicturesAndSize(
      int num_pictures, Size& picture_size)
};

By doing so, we don't need the "kAllocateNewSurfaces" return code.

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
File content/common/gpu/media/vp8_decoder.cc (right):

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/vp8_decoder.cc:69: if (pic_size_.IsEmpty() ||
new_pic_size != pic_size_) {
Do we need to test pic_size_.IsEmpty() ?

Since new_pic_size won't be empty, the above check seems redundant.

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/vp8_decoder.cc:111: DCHECK(last_frame_);
Why it is not a decode error? I mean a bad stream should not crash chrome.

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/vp8_decoder.cc:116: DCHECK(alt_frame_);
Same here.

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/vp8_decoder.cc:160: BeginFrame();
[optional] I am kind of thinking there is no need to extract BiginFrame() as an
extra function. It is not so complicated.

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/vp8_decoder.cc:180: static const size_t kPicsInPipeline
= media::limits::kMaxVideoFrames + 2;
remove static

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
File content/common/gpu/media/vp8_decoder.h (right):

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/vp8_decoder.h:37:
DISALLOW_COPY_AND_ASSIGN(VP8Accelerator);
put under private:.

https://codereview.chromium.org/833063003/diff/60001/content/common/gpu/media...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.h (right):

https://codereview.chromium.org/833063003/diff/60001/content/common/gpu/media...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:58: std::string
ToString() const;
const std::string&

https://codereview.chromium.org/833063003/diff/60001/content/common/gpu/media...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:95: bool
SubmitSlice(int index, const uint8_t* data, size_t size);
I am confused, which one feed the input to this class ?
SubmitSlice() or Decode()

https://codereview.chromium.org/833063003/diff/60001/content/common/gpu/media...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:474:
V4L2H264Picture* AsV4L2H264Picture() override { return this; }
Why we need this ? What it overrides, there is no such function in H264Picture.

[kcwu, 2015-01-09 10:07:48]

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/acc...
File content/common/gpu/media/accelerated_video_decoder.h (right):

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/acc...
content/common/gpu/media/accelerated_video_decoder.h:32: // Stop (pause)
decoding, discarding all remaining inputs and outputs,
Make this first line.

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/acc...
content/common/gpu/media/accelerated_video_decoder.h:40: // TODO posciak:
unsupported streams are currently treated as error
TODO(posciak):

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/acc...
content/common/gpu/media/accelerated_video_decoder.h:57: virtual gfx::Size
GetPicSize() = 0;
On 2015/01/08 15:42:06, Owen Lin wrote:
> return "const gfx::Size&" ?

GetPicSize() const = 0;

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/acc...
content/common/gpu/media/accelerated_video_decoder.h:58: virtual size_t
GetRequiredNumOfPictures() = 0;
...() const = 0;

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
File content/common/gpu/media/vp8_decoder.cc (right):

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/vp8_decoder.cc:60: return VP8Decoder::kDecodeError;
Maybe it is just not enough data, not really error.

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/vp8_decoder.cc:69: if (pic_size_.IsEmpty() ||
new_pic_size != pic_size_) {
"new_pic_size != pic_size_" is enough.

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/vp8_decoder.cc:91: return kRanOutOfStreamData;
IIUC, you only decoded one frame and there may be still enough stream data
remain, right?

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/vp8_decoder.cc:174: frame_size_ = 0;
maybe some data still remain after current frame?

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/vp8_decoder.cc:179: static const size_t
kVP8NumFramesActive = 4;
no need "static"

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
File content/common/gpu/media/vp8_decoder.h (right):

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/vp8_decoder.h:37:
DISALLOW_COPY_AND_ASSIGN(VP8Accelerator);
private: before this line

[kcwu, 2015-01-09 11:03:00]

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

https://codereview.chromium.org/833063003/diff/80001/content/common/gpu/media...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1030:
decoder_current_bitstream_buffer_.reset();
Will decoder cache remain data in current buffer? Maybe the decoder needs data
in both current and next bitstream to decode one frame?

[Pawel Osciak, 2015-01-09 13:50:31]

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/acc...
File content/common/gpu/media/accelerated_video_decoder.h (right):

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/acc...
content/common/gpu/media/accelerated_video_decoder.h:52: virtual DecResult
Decode() WARN_UNUSED_RESULT = 0;
On 2015/01/08 15:42:06, Owen Lin wrote:
> How is the decoded frames be returned?

Via a separate callback. I don't have a generalized callback here in this
interface, because some decoders call Output(H264Picture) and some
Output(VP8Picture) and I haven't unified these classes enough. Perhaps in a
further redesign...

https://codereview.chromium.org/833063003/diff/1/content/common/gpu/media/acc...
content/common/gpu/media/accelerated_video_decoder.h:52: virtual DecResult
Decode() WARN_UNUSED_RESULT = 0;
On 2015/01/08 15:42:06, Owen Lin wrote:
> Is this a blocking call? 
> How many frame will be decoded? If not one, what happens if user want to seek
or
> stop the video?

As many as there are in the buffer. The frames will be returned to client for
output via a callback before this returns, but the client may decide to drop
them if there is a seek. For stop, it will have to be after this bitstream
buffer.

But actually, we will always in practice get one frame per buffer in Chrome

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
File content/common/gpu/media/accelerated_video_decoder.h (right):

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/accelerated_video_decoder.h:20: class CONTENT_EXPORT
AcceleratedVideoDecoder {
On 2015/01/09 09:56:14, Owen Lin wrote:
> I didn't see how the decoded frames be returned in this class. After reading
the
> other parts, I realize the frame ares actually output by different
Accelerators.
> This sounds wired. I think it would be nice if we can have an interface here.
By
> doing so, the client of this class don't need to deal with different so called
> Accelerators.
> 
> My proposal is adding:
> 
>   bool GetDecodedPicture(Picture* picture);
> 
> to this class, and change the OutputPicture of those Accelerators to actually
to
> export from the internal format to media::Picture.
> e.g., bool Vp8Accelerator::OutputPicture(Vp8Picture in, media::Picture* out);
> 
> But this is just a rough thought.


I'm also not happy with this detail, I agree. I was considering a solution like
this, but I wasn't exactly happy with it either.

I know it's weird to output from Accelerator, but it's a simplification,
otherwise we'd need to like you say use something abstract, like Picture. That
would have been fine and I'd like that, but the reality is that I need to get
the underlying surface from Picture anyway to be able to output, because I need
to wait for it to finish decoding and also I need to keep reference to it while
it's in the client. And conversion from Picture to surface would require an
additional map, which I wanted to avoid.

I still want to do something about this, but it's more complicated than it
looks. I want to take another look at the design a bit later once we have a good
idea how to solve this.

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/accelerated_video_decoder.h:57: virtual gfx::Size
GetPicSize() = 0;
On 2015/01/09 09:56:14, Owen Lin wrote:
> Would it be better to move into those Accelerator?
> 
> class Vp8Accelerator {
>   int SetRequiredNumOfPicturesAndSize(
>       int num_pictures, Size& picture_size)
> };
> 
> By doing so, we don't need the "kAllocateNewSurfaces" return code.

We still need to return from Decode(), because we cannot continue decoding with
old surfaces. So with that, the accelerator would have to know that something
called SetRequiredNumOfPicturesAndSize before this returned. Of course that
would have been fine, but there is another complication that accelerator would
have to store the result of SetRequiredNumOfPicturesAndSize until next time
Decode returned and would have to check whether that happened every time Decode
returned, even if it didn't.

With a return, it's easier to know what's going on, and we need to return from
Decode() anyway, and can actually pull the information when we decide to change
surface set, and not have it forced onto us.

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
File content/common/gpu/media/vp8_decoder.cc (right):

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/vp8_decoder.cc:60: return VP8Decoder::kDecodeError;
On 2015/01/09 10:07:48, kcwu wrote:
> Maybe it is just not enough data, not really error.

This would indicate a partial frame (since frame_size_ > 0) and we don't allow
that.

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/vp8_decoder.cc:91: return kRanOutOfStreamData;
On 2015/01/09 10:07:48, kcwu wrote:
> IIUC, you only decoded one frame and there may be still enough stream data
> remain, right?

Hm, true, it doesn't happen now since Chrome segments into frames always... I
should loop though anyway to keep this universal as intended.

https://codereview.chromium.org/833063003/diff/20001/content/common/gpu/media...
content/common/gpu/media/vp8_decoder.cc:111: DCHECK(last_frame_);
On 2015/01/09 09:56:14, Owen Lin wrote:
> Why it is not a decode error? I mean a bad stream should not crash chrome.

Ah good point.

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

https://codereview.chromium.org/833063003/diff/80001/content/common/gpu/media...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1030:
decoder_current_bitstream_buffer_.reset();
On 2015/01/09 11:03:00, kcwu wrote:
> Will decoder cache remain data in current buffer? Maybe the decoder needs data
> in both current and next bitstream to decode one frame?

The VDA will always get a full NALU (H264) or frame (VP8), decoder keeps data
across calls as long as the NALUs are not partial.

[kcwu, 2015-01-09 15:36:46]

https://codereview.chromium.org/833063003/diff/60001/content/common/gpu/media...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.h (right):

https://codereview.chromium.org/833063003/diff/60001/content/common/gpu/media...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:198:
kInputBufferMaxSizeFor1080p = 1024 * 1024,
const size_t, not enum.

https://codereview.chromium.org/833063003/diff/60001/content/common/gpu/media...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:304:
kFlushBufferId = -2  // Buffer id for flush buffer, queued by FlushTask().
Give -1 a name as well. Otherwise -2 looks weird.

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
File content/common/gpu/media/h264_decoder.h (right):

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
content/common/gpu/media/h264_decoder.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
2015

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
File content/common/gpu/media/h264_dpb.h (right):

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.h:82: DISALLOW_COPY_AND_ASSIGN(H264Picture);
private:

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:417:
fmtdesc.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
add output_format_fourcc_=0 or DCHECK_EQ here to make it more obvious.

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
File content/common/gpu/media/vp8_picture.h (right):

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
content/common/gpu/media/vp8_picture.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
2015

[kcwu, 2015-01-09 19:56:22]

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:526: << ",
width=" << frame_buffer_size_.width()
use frame_buffer_size_.ToString() ?

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:995: const
uint8* const data = reinterpret_cast<const uint8*>(
uint8_t

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1276: DVLOGF(3)
<< "got picture id= " << picture_buffer_id
no space after =

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1475: for (auto&
pic : src_pic_list)
DCHECK_LE(src_pic_list.size(), 32)

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1487: for (const
auto& pic : dpb) {
DCHECK_LE(dbp.size(), arrarysize(v4l2_decode_param_.dpb));

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1537:
COMPILE_ASSERT(arraysize(v4l2_sps.offset_for_ref_frame) ==
static_assert

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1611:
COMPILE_ASSERT(arraysize(v4l2_scaling_matrix.scaling_list_4x4) <=
static_assert

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1795:
memcpy(static_cast<uint8*>(input_record.address) + input_record.bytes_used,
uint8_t

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.h (right):

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:37:
V4L2DecodeSurface(int32 bitstream_id,
I found you use int32 and uint32_t -- one with _t and one without. Are you
intended?

https://codereview.chromium.org/833063003/diff/100001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:129: uint8_t
dst_list[32]);
would be good if 32 is const variable. and better with comment.

[wuchengli, 2015-01-10 02:20:03]

Add BUG and TEST. Also update the title now that this adds v4l2 slice vda.

[Pawel Osciak, 2015-01-10 04:40:53]

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

[Pawel Osciak, 2015-01-10 04:40:54]

xhwang@chromium.org: Could you please OWNERS the two-liner in media/ please?
Thanks.

[Pawel Osciak, 2015-01-12 03:36:06]

updated, ptal

[Pawel Osciak, 2015-01-12 07:18:20]

All previous comments addressed.

https://chromiumcodereview.appspot.com/833063003/diff/1/content/common/gpu/me...
File content/common/gpu/media/accelerated_video_decoder.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/1/content/common/gpu/me...
content/common/gpu/media/accelerated_video_decoder.h:10: #include
"base/memory/ref_counted.h"
On 2015/01/08 15:42:06, Owen Lin wrote:
> not used for the above includes?

Done.

https://chromiumcodereview.appspot.com/833063003/diff/1/content/common/gpu/me...
content/common/gpu/media/accelerated_video_decoder.h:32: // Stop (pause)
decoding, discarding all remaining inputs and outputs,
On 2015/01/09 10:07:47, kcwu wrote:
> Make this first line.

Done.

https://chromiumcodereview.appspot.com/833063003/diff/1/content/common/gpu/me...
content/common/gpu/media/accelerated_video_decoder.h:38: enum DecResult {
On 2015/01/08 15:42:06, Owen Lin wrote:
> Use a full name, e.g., DecodeResult ?

Done.

https://chromiumcodereview.appspot.com/833063003/diff/1/content/common/gpu/me...
content/common/gpu/media/accelerated_video_decoder.h:40: // TODO posciak:
unsupported streams are currently treated as error
On 2015/01/09 10:07:47, kcwu wrote:
> TODO(posciak):

Done.

https://chromiumcodereview.appspot.com/833063003/diff/1/content/common/gpu/me...
content/common/gpu/media/accelerated_video_decoder.h:57: virtual gfx::Size
GetPicSize() = 0;
On 2015/01/09 10:07:47, kcwu wrote:
> On 2015/01/08 15:42:06, Owen Lin wrote:
> > return "const gfx::Size&" ?
> 
> GetPicSize() const = 0;

Done.

https://chromiumcodereview.appspot.com/833063003/diff/1/content/common/gpu/me...
content/common/gpu/media/accelerated_video_decoder.h:57: virtual gfx::Size
GetPicSize() = 0;
On 2015/01/08 15:42:06, Owen Lin wrote:
> return "const gfx::Size&" ?

Done.

https://chromiumcodereview.appspot.com/833063003/diff/1/content/common/gpu/me...
content/common/gpu/media/accelerated_video_decoder.h:58: virtual size_t
GetRequiredNumOfPictures() = 0;
On 2015/01/09 10:07:47, kcwu wrote:
> ...() const = 0;

Done.

https://chromiumcodereview.appspot.com/833063003/diff/1/content/common/gpu/me...
content/common/gpu/media/accelerated_video_decoder.h:60:
DISALLOW_COPY_AND_ASSIGN(AcceleratedVideoDecoder);
On 2015/01/08 15:42:06, Owen Lin wrote:
> You have put this under private: to make it works.

Done.

https://chromiumcodereview.appspot.com/833063003/diff/1/content/common/gpu/me...
File content/common/gpu/media/h264_decoder.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/1/content/common/gpu/me...
content/common/gpu/media/h264_decoder.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2015/01/08 15:42:06, Owen Lin wrote:
> 2015? 
> Remove (c)?

This is basically a copy of vaapi_h264_decoder, so keeping the copyright from
the old file.

https://chromiumcodereview.appspot.com/833063003/diff/1/content/common/gpu/me...
content/common/gpu/media/h264_decoder.h:11: #include "base/memory/linked_ptr.h"
On 2015/01/08 15:42:06, Owen Lin wrote:
> Are the above two includes not used?

Done.

https://chromiumcodereview.appspot.com/833063003/diff/1/content/common/gpu/me...
content/common/gpu/media/h264_decoder.h:55: virtual bool SubmitDecode(const
scoped_refptr<H264Picture>& pic) = 0;
On 2015/01/08 15:42:06, Owen Lin wrote:
> Add docs. I cannot figure out how these function work, it seems not trivial.

Done.

https://chromiumcodereview.appspot.com/833063003/diff/1/content/common/gpu/me...
content/common/gpu/media/h264_decoder.h:58:
DISALLOW_COPY_AND_ASSIGN(H264Accelerator);
On 2015/01/08 15:42:06, Owen Lin wrote:
> put it under "private:"

Done.

https://chromiumcodereview.appspot.com/833063003/diff/20001/content/common/gp...
File content/common/gpu/media/vp8_decoder.cc (right):

https://chromiumcodereview.appspot.com/833063003/diff/20001/content/common/gp...
content/common/gpu/media/vp8_decoder.cc:69: if (pic_size_.IsEmpty() ||
new_pic_size != pic_size_) {
On 2015/01/09 10:07:48, kcwu wrote:
> "new_pic_size != pic_size_" is enough.

Done.

https://chromiumcodereview.appspot.com/833063003/diff/20001/content/common/gp...
content/common/gpu/media/vp8_decoder.cc:69: if (pic_size_.IsEmpty() ||
new_pic_size != pic_size_) {
On 2015/01/09 09:56:14, Owen Lin wrote:
> Do we need to test pic_size_.IsEmpty() ?
> 
> Since new_pic_size won't be empty, the above check seems redundant. 

Done.

https://chromiumcodereview.appspot.com/833063003/diff/20001/content/common/gp...
content/common/gpu/media/vp8_decoder.cc:91: return kRanOutOfStreamData;
On 2015/01/09 10:07:48, kcwu wrote:
> IIUC, you only decoded one frame and there may be still enough stream data
> remain, right?

Actually, this would apply to H264, but for VP8 we have no way of knowing what
is the frame size from the raw stream, we must get it from the client, so we
can't use any remaining data.

https://chromiumcodereview.appspot.com/833063003/diff/20001/content/common/gp...
content/common/gpu/media/vp8_decoder.cc:111: DCHECK(last_frame_);
On 2015/01/09 13:50:31, Pawel Osciak wrote:
> On 2015/01/09 09:56:14, Owen Lin wrote:
> > Why it is not a decode error? I mean a bad stream should not crash chrome.
> 
> Ah good point.

Well, actually, recalling my original line of thought, it can't happen unless we
have a bug. Either it's a key frame, so we set all three references, or it's not
and we replace them. We never drop them, unless it's a key frame.

https://chromiumcodereview.appspot.com/833063003/diff/20001/content/common/gp...
content/common/gpu/media/vp8_decoder.cc:116: DCHECK(alt_frame_);
On 2015/01/09 09:56:14, Owen Lin wrote:
> Same here.

As above.

https://chromiumcodereview.appspot.com/833063003/diff/20001/content/common/gp...
content/common/gpu/media/vp8_decoder.cc:160: BeginFrame();
On 2015/01/09 09:56:14, Owen Lin wrote:
> [optional] I am kind of thinking there is no need to extract BiginFrame() as
an
> extra function. It is not so complicated.

Done.

https://chromiumcodereview.appspot.com/833063003/diff/20001/content/common/gp...
content/common/gpu/media/vp8_decoder.cc:174: frame_size_ = 0;
On 2015/01/09 10:07:48, kcwu wrote:
> maybe some data still remain after current frame?

Actually we can't use it, since we must be given frame size from outside, we
have no way to get the size from the stream itself (there is no container in
what we get).

https://chromiumcodereview.appspot.com/833063003/diff/20001/content/common/gp...
content/common/gpu/media/vp8_decoder.cc:179: static const size_t
kVP8NumFramesActive = 4;
On 2015/01/09 10:07:48, kcwu wrote:
> no need "static"

Done.

https://chromiumcodereview.appspot.com/833063003/diff/20001/content/common/gp...
content/common/gpu/media/vp8_decoder.cc:180: static const size_t kPicsInPipeline
= media::limits::kMaxVideoFrames + 2;
On 2015/01/09 09:56:14, Owen Lin wrote:
> remove static

Done.

https://chromiumcodereview.appspot.com/833063003/diff/20001/content/common/gp...
File content/common/gpu/media/vp8_decoder.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/20001/content/common/gp...
content/common/gpu/media/vp8_decoder.h:37:
DISALLOW_COPY_AND_ASSIGN(VP8Accelerator);
On 2015/01/09 09:56:14, Owen Lin wrote:
> put under private:.

Done.

https://chromiumcodereview.appspot.com/833063003/diff/20001/content/common/gp...
content/common/gpu/media/vp8_decoder.h:37:
DISALLOW_COPY_AND_ASSIGN(VP8Accelerator);
On 2015/01/09 10:07:48, kcwu wrote:
> private: before this line

Done.

https://chromiumcodereview.appspot.com/833063003/diff/60001/content/common/gp...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/60001/content/common/gp...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:58: std::string
ToString() const;
On 2015/01/09 09:56:14, Owen Lin wrote:
> const std::string&

I'm not sure we want a reference, the string is local to the function.

https://chromiumcodereview.appspot.com/833063003/diff/60001/content/common/gp...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:95: bool
SubmitSlice(int index, const uint8_t* data, size_t size);
On 2015/01/09 09:56:14, Owen Lin wrote:
> I am confused, which one feed the input to this class ?
> SubmitSlice() or Decode()

Added docs.

https://chromiumcodereview.appspot.com/833063003/diff/60001/content/common/gp...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:198:
kInputBufferMaxSizeFor1080p = 1024 * 1024,
On 2015/01/09 15:36:46, kcwu wrote:
> const size_t, not enum.

Done.

https://chromiumcodereview.appspot.com/833063003/diff/60001/content/common/gp...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:304:
kFlushBufferId = -2  // Buffer id for flush buffer, queued by FlushTask().
On 2015/01/09 15:36:46, kcwu wrote:
> Give -1 a name as well. Otherwise -2 looks weird.

Done.

https://chromiumcodereview.appspot.com/833063003/diff/60001/content/common/gp...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:474:
V4L2H264Picture* AsV4L2H264Picture() override { return this; }
On 2015/01/09 09:56:14, Owen Lin wrote:
> Why we need this ? What it overrides, there is no such function in
H264Picture.

There is one, sorry didn't upload the file for previous review.

https://chromiumcodereview.appspot.com/833063003/diff/80001/content/common/gp...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/833063003/diff/80001/content/common/gp...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1030:
decoder_current_bitstream_buffer_.reset();
On 2015/01/09 13:50:31, Pawel Osciak wrote:
> On 2015/01/09 11:03:00, kcwu wrote:
> > Will decoder cache remain data in current buffer? Maybe the decoder needs
data
> > in both current and next bitstream to decode one frame?
> 
> The VDA will always get a full NALU (H264) or frame (VP8), decoder keeps data
> across calls as long as the NALUs are not partial.

The above applies to H264. For VP8, we have no way of knowing the size of frame
from stream, so we rely on client to pass it and have to discard any remaining
data after the first frame.

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
File content/common/gpu/media/h264_decoder.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
content/common/gpu/media/h264_decoder.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2015/01/09 15:36:46, kcwu wrote:
> 2015

This is a modified file from 2012.

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
File content/common/gpu/media/h264_dpb.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
content/common/gpu/media/h264_dpb.h:82: DISALLOW_COPY_AND_ASSIGN(H264Picture);
On 2015/01/09 15:36:46, kcwu wrote:
> private:

Done.

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:417:
fmtdesc.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
On 2015/01/09 15:36:46, kcwu wrote:
> add output_format_fourcc_=0 or DCHECK_EQ here to make it more obvious.

Done.

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:526: << ",
width=" << frame_buffer_size_.width()
On 2015/01/09 19:56:22, kcwu wrote:
> use frame_buffer_size_.ToString() ?

Done.

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:995: const
uint8* const data = reinterpret_cast<const uint8*>(
On 2015/01/09 19:56:21, kcwu wrote:
> uint8_t

Done.

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1276: DVLOGF(3)
<< "got picture id= " << picture_buffer_id
On 2015/01/09 19:56:21, kcwu wrote:
> no space after =

Done.

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1475: for (auto&
pic : src_pic_list)
On 2015/01/09 19:56:21, kcwu wrote:
> DCHECK_LE(src_pic_list.size(), 32)


Uhh good catch.

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1487: for (const
auto& pic : dpb) {
On 2015/01/09 19:56:21, kcwu wrote:
> DCHECK_LE(dbp.size(), arrarysize(v4l2_decode_param_.dpb));

Thank you.

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1537:
COMPILE_ASSERT(arraysize(v4l2_sps.offset_for_ref_frame) ==
On 2015/01/09 19:56:22, kcwu wrote:
> static_assert

Done.

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1611:
COMPILE_ASSERT(arraysize(v4l2_scaling_matrix.scaling_list_4x4) <=
On 2015/01/09 19:56:21, kcwu wrote:
> static_assert

Done.

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1795:
memcpy(static_cast<uint8*>(input_record.address) + input_record.bytes_used,
On 2015/01/09 19:56:22, kcwu wrote:
> uint8_t

Done.

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:37:
V4L2DecodeSurface(int32 bitstream_id,
On 2015/01/09 19:56:22, kcwu wrote:
> I found you use int32 and uint32_t -- one with _t and one without. Are you
> intended?

Yes, BitstreamBuffer still uses int32.

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:129: uint8_t
dst_list[32]);
On 2015/01/09 19:56:22, kcwu wrote:
> would be good if 32 is const variable. and better with comment.

Done.

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
File content/common/gpu/media/vp8_picture.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/100001/content/common/g...
content/common/gpu/media/vp8_picture.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/01/09 15:36:46, kcwu wrote:
> 2015

Done.

[kcwu, 2015-01-12 07:31:46]

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/common/g...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:26: #define
DVLOGF(level) LOG(ERROR) << __FUNCTION__ << "(): "
level is ignored?

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:471:
buffer.length   = input_planes_count_;
I think we usually don't align assignment?

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:529:
output_buffer_map_.resize(reqbufs.count);
according to http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-reqbufs.html
reqbufs.count may be larger than output_dpb_size_.
But I don't fully understand that case. 

I'm not sure. Should this line be resize(reqbufs.count) or
resize(output_dpb_size_)?

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1057:
DCHECK_EQ(state_, kDecoding);
code duped

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/common/g...
File content/common/gpu/media/vp8_picture.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/common/g...
content/common/gpu/media/vp8_picture.h:8: //#include
"media/filters/vp8_parser.h"
?

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/content_...
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/content_...
content/content_common.gypi:856: 'common/gpu/media/vaapi_h264_dpb.cc',
sort

[kcwu, 2015-01-12 13:22:01]

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/h264_decoder.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/h264_decoder.h:49: // Note that his does not run decode
in the accelerator and the decoder
s/his/this/

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

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:25: #define
LOGF(level) LOG(level) << __FUNCTION__ << "(): "
Be consistent to use __FUNCTION__ or __func__ in this file.

BTW, __func__ is banned according to https://chromium-cpp.appspot.com/ , due to
MSVC though. I'm fine to use either, just be consistent.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1157:
DCHECK(surfaces_at_display_.size() + free_output_buffers_.size() ==
DCHECK_EQ

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1209:
DCHECK(buffers[i].size() == frame_buffer_size_);
DCHECK_EQ

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1490:
scoped_refptr<H264Picture> pic = src_pic_list[i];
const scoped_refptr<H264Picture>& or const auto&
otherwise it will produce unnecessary AddRef() and Release()

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1567: #define
SET_V4L2_SPS_FLAG_IF(cond, flag) \
Do we really need this macro? I feel there is not much duplication.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:2197: while
(pending_picture_ready_.size() > 0) {
!empty()

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:339: const int
kFlushBufferId = -1;
I'm confused. IIUC, you already used -1 for something like invalid id and -2 for
flush. Now you use -1 for flush, won't it conflict with other -1?

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/content_...
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/content_...
content/content_common.gypi:841:
'common/gpu/media/v4l2_slice_video_decode_accelerator.h',
sort

[scherkus (not reviewing), 2015-01-13 01:24:58]

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

[scherkus (not reviewing), 2015-01-13 01:25:01]

can you provide some details on what necessitated making everything reference
counted?

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/generic_v4l2_video_device.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/generic_v4l2_video_device.h:45: virtual
~GenericV4L2Device();
override

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/h264_decoder.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/h264_decoder.h:97: virtual ~H264Decoder();
override

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

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/h264_dpb.cc:40: for (H264Picture::Vector::iterator it =
pics_.begin();
auto& ?

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/h264_dpb.cc:52: for (H264Picture::Vector::iterator it =
pics_.begin(); it != pics_.end(); ) {
ditto

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/h264_dpb.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/h264_dpb.h:73: H264Picture() {}
move impl to .cc

see http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/h264_dpb.h:75: virtual V4L2H264Picture*
AsV4L2H264Picture() { return nullptr; }
is it ever reasonable to construct an H264Picture as opposed to a subclass?

if it isn't, I would make this pure virtual

also .. why not just stick this pure virtual method on H264PictureBase? at least
as far as I can tell I don't see any other subclasses of H264PictureBase

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/h264_dpb.h:81: virtual ~H264Picture() {}
override

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/tegra_v4l2_video_device.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/tegra_v4l2_video_device.h:48: virtual
~TegraV4L2Device();
override

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/v4l2_image_processor.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_image_processor.h:26: explicit
V4L2ImageProcessor(scoped_refptr<V4L2Device> device);
const &

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:33: class
V4L2DecodeSurface : public base::RefCounted<V4L2DecodeSurface> {
is it possible to fwd decl this and move to the .cc simplify this header file?

see
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-in....
for details

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:81: virtual
~V4L2SliceVideoDecodeAccelerator();
override

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:84: virtual bool
Initialize(media::VideoCodecProfile profile,
remove all virtual keywords

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:96: class
V4L2H264Accelerator : public H264Decoder::H264Accelerator {
is it possible to fwd decl this and move to the .cc simplify this header file?

see
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-in....
for details

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:99: virtual
~V4L2H264Accelerator() {}
override and move impl to .cc

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:501: class
V4L2H264Picture : public H264Picture {
ditto for .cc

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:521: class
V4L2VP8Picture : public VP8Picture {
ditto for .cc

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

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:161:
scoped_refptr<V4L2Device> device,
const ref

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/v4l2_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_video_decode_accelerator.h:84:
scoped_refptr<V4L2Device> device,
const ref

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/v4l2_video_device.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_video_device.h:104: virtual ~V4L2Device();
override

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/vp8_decoder.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/vp8_decoder.h:62: virtual ~VP8Decoder();
override

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/vp8_picture.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/vp8_picture.h:16: VP8Picture() {}
move impl to .cc

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/vp8_picture.h:22: virtual ~VP8Picture() {}
override and move impl to .cc

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/content_...
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/content_...
content/content_common.gypi:857: 'common/gpu/media/vaapi_h264_dpb.h',
also needs sorting

[Pawel Osciak, 2015-01-13 11:33:35]

done. ptal.

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/common/g...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:26: #define
DVLOGF(level) LOG(ERROR) << __FUNCTION__ << "(): "
On 2015/01/12 07:31:46, kcwu wrote:
> level is ignored?

Just for debugging, sorry.

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:471:
buffer.length   = input_planes_count_;
On 2015/01/12 07:31:46, kcwu wrote:
> I think we usually don't align assignment?

Yup, sorry.

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:529:
output_buffer_map_.resize(reqbufs.count);
On 2015/01/12 07:31:46, kcwu wrote:
> according to http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-reqbufs.html
> reqbufs.count may be larger than output_dpb_size_.
> But I don't fully understand that case. 
> 
> I'm not sure. Should this line be resize(reqbufs.count) or
> resize(output_dpb_size_)?

In l.524 we check that we got not less than output_dpb_size_, but if we got
more, we can use more. We don't map output buffers directly to dpb.

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1057:
DCHECK_EQ(state_, kDecoding);
On 2015/01/12 07:31:46, kcwu wrote:
> code duped

Done.

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/common/g...
File content/common/gpu/media/vp8_picture.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/common/g...
content/common/gpu/media/vp8_picture.h:8: //#include
"media/filters/vp8_parser.h"
On 2015/01/12 07:31:46, kcwu wrote:
> ?

Removed in the next PS.

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/content_...
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/833063003/diff/120001/content/content_...
content/content_common.gypi:856: 'common/gpu/media/vaapi_h264_dpb.cc',
On 2015/01/12 07:31:46, kcwu wrote:
> sort

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/generic_v4l2_video_device.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/generic_v4l2_video_device.h:45: virtual
~GenericV4L2Device();
On 2015/01/13 01:24:59, scherkus wrote:
> override

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/h264_decoder.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/h264_decoder.h:49: // Note that his does not run decode
in the accelerator and the decoder
On 2015/01/12 13:22:00, kcwu wrote:
> s/his/this/

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/h264_decoder.h:97: virtual ~H264Decoder();
On 2015/01/13 01:24:59, scherkus wrote:
> override

Done.

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

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/h264_dpb.cc:40: for (H264Picture::Vector::iterator it =
pics_.begin();
On 2015/01/13 01:24:59, scherkus wrote:
> auto& ?

I need the iterator to erase().

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/h264_dpb.cc:52: for (H264Picture::Vector::iterator it =
pics_.begin(); it != pics_.end(); ) {
On 2015/01/13 01:24:59, scherkus wrote:
> ditto

Ditto :)

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/h264_dpb.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/h264_dpb.h:73: H264Picture() {}
On 2015/01/13 01:24:59, scherkus wrote:
> move impl to .cc
> 
> see http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/h264_dpb.h:75: virtual V4L2H264Picture*
AsV4L2H264Picture() { return nullptr; }
On 2015/01/13 01:24:59, scherkus wrote:
> is it ever reasonable to construct an H264Picture as opposed to a subclass?

Yes, if we intend to use H264Decoder without hiding any implementation details
in H264Picture.

> if it isn't, I would make this pure virtual
> 
> also .. why not just stick this pure virtual method on H264PictureBase? at
least
> as far as I can tell I don't see any other subclasses of H264PictureBase

The only reason for the ugly existence of H264PictureBase was that I wanted to
have a memset(this,0,...) constructor for it. I'm not aware of any other method
to do this...

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/h264_dpb.h:81: virtual ~H264Picture() {}
On 2015/01/13 01:24:59, scherkus wrote:
> override

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/tegra_v4l2_video_device.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/tegra_v4l2_video_device.h:48: virtual
~TegraV4L2Device();
On 2015/01/13 01:24:59, scherkus wrote:
> override

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/v4l2_image_processor.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_image_processor.h:26: explicit
V4L2ImageProcessor(scoped_refptr<V4L2Device> device);
On 2015/01/13 01:25:00, scherkus wrote:
> const &

Done.

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

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:25: #define
LOGF(level) LOG(level) << __FUNCTION__ << "(): "
On 2015/01/12 13:22:01, kcwu wrote:
> Be consistent to use __FUNCTION__ or __func__ in this file.
> 
> BTW, __func__ is banned according to https://chromium-cpp.appspot.com/ , due
to
> MSVC though. I'm fine to use either, just be consistent.

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1157:
DCHECK(surfaces_at_display_.size() + free_output_buffers_.size() ==
On 2015/01/12 13:22:01, kcwu wrote:
> DCHECK_EQ

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1209:
DCHECK(buffers[i].size() == frame_buffer_size_);
On 2015/01/12 13:22:01, kcwu wrote:
> DCHECK_EQ

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1490:
scoped_refptr<H264Picture> pic = src_pic_list[i];
On 2015/01/12 13:22:01, kcwu wrote:
> const scoped_refptr<H264Picture>& or const auto&
> otherwise it will produce unnecessary AddRef() and Release()

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1567: #define
SET_V4L2_SPS_FLAG_IF(cond, flag) \
On 2015/01/12 13:22:01, kcwu wrote:
> Do we really need this macro? I feel there is not much duplication.

It's really more readable than without it.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:2197: while
(pending_picture_ready_.size() > 0) {
On 2015/01/12 13:22:01, kcwu wrote:
> !empty()

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:33: class
V4L2DecodeSurface : public base::RefCounted<V4L2DecodeSurface> {
On 2015/01/13 01:25:00, scherkus wrote:
> is it possible to fwd decl this and move to the .cc simplify this header file?
> 
> see
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-in....
> for details

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:81: virtual
~V4L2SliceVideoDecodeAccelerator();
On 2015/01/13 01:25:00, scherkus wrote:
> override

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:84: virtual bool
Initialize(media::VideoCodecProfile profile,
On 2015/01/13 01:25:00, scherkus wrote:
> remove all virtual keywords

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:96: class
V4L2H264Accelerator : public H264Decoder::H264Accelerator {
On 2015/01/13 01:25:00, scherkus wrote:
> is it possible to fwd decl this and move to the .cc simplify this header file?
> 
> see
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-in....
> for details

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:99: virtual
~V4L2H264Accelerator() {}
On 2015/01/13 01:25:00, scherkus wrote:
> override and move impl to .cc

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:339: const int
kFlushBufferId = -1;
On 2015/01/12 13:22:01, kcwu wrote:
> I'm confused. IIUC, you already used -1 for something like invalid id and -2
for
> flush. Now you use -1 for flush, won't it conflict with other -1?

I'm confused what happened here too. Sorry.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:501: class
V4L2H264Picture : public H264Picture {
On 2015/01/13 01:25:00, scherkus wrote:
> ditto for .cc

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:521: class
V4L2VP8Picture : public VP8Picture {
On 2015/01/13 01:25:00, scherkus wrote:
> ditto for .cc

Done.

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

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:161:
scoped_refptr<V4L2Device> device,
On 2015/01/13 01:25:00, scherkus wrote:
> const ref

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/v4l2_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_video_decode_accelerator.h:84:
scoped_refptr<V4L2Device> device,
On 2015/01/13 01:25:00, scherkus wrote:
> const ref

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/v4l2_video_device.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_video_device.h:104: virtual ~V4L2Device();
On 2015/01/13 01:25:00, scherkus wrote:
> override

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/vp8_decoder.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/vp8_decoder.h:62: virtual ~VP8Decoder();
On 2015/01/13 01:25:00, scherkus wrote:
> override

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/vp8_picture.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/vp8_picture.h:16: VP8Picture() {}
On 2015/01/13 01:25:00, scherkus wrote:
> move impl to .cc

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/vp8_picture.h:22: virtual ~VP8Picture() {}
On 2015/01/13 01:25:00, scherkus wrote:
> override and move impl to .cc

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/content_...
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/content_...
content/content_common.gypi:841:
'common/gpu/media/v4l2_slice_video_decode_accelerator.h',
On 2015/01/12 13:22:01, kcwu wrote:
> sort

Done.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/content_...
content/content_common.gypi:857: 'common/gpu/media/vaapi_h264_dpb.h',
On 2015/01/13 01:25:00, scherkus wrote:
> also needs sorting

Done.

[Pawel Osciak, 2015-01-13 11:44:34]

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

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1209:
DCHECK(buffers[i].size() == frame_buffer_size_);
On 2015/01/13 11:33:34, Pawel Osciak wrote:
> On 2015/01/12 13:22:01, kcwu wrote:
> > DCHECK_EQ
> 
> Done.

Actually, EQ doesn't compile, probably can't use operator==.

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/v4l2_video_device.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/v4l2_video_device.h:104: virtual ~V4L2Device();
On 2015/01/13 11:33:35, Pawel Osciak wrote:
> On 2015/01/13 01:25:00, scherkus wrote:
> > override
> 
> Done.

Actually, I don't think I can "override" non-virtual destructor of RefCounted?

[kcwu, 2015-01-13 12:09:40]

lgtm

nits

https://chromiumcodereview.appspot.com/833063003/diff/160001/content/common/g...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/833063003/diff/160001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:146: :
input_id(-1),
give all -1 a name.

https://chromiumcodereview.appspot.com/833063003/diff/160001/content/common/g...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:156:
picture_id(-1),
ditto.

https://chromiumcodereview.appspot.com/833063003/diff/160001/content/content_...
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/833063003/diff/160001/content/content_...
content/content_common.gypi:844: 'common/gpu/media/vp8_picture.cc',
sort

[Pawel Osciak, 2015-01-13 13:35:10]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2015-01-13 13:35:18]

The CQ bit was unchecked by posciak@chromium.org

[Pawel Osciak, 2015-01-13 13:51:17]

Patchset #9 (id:180001) has been deleted

[Pawel Osciak, 2015-01-14 00:29:16]

On 2015/01/13 01:25:01, scherkus wrote:
> can you provide some details on what necessitated making everything reference
> counted?
> 

The lifetime of a surface is complicated and the path of state transitions
depends on the stream, timings, codec hardware, display delay, etc.

A surface is first passed to a Decoder, which decodes into it (ref), passing it
to Accelerator when ready, which schedules it to be decoded in HW (this needs a
ref because we do HW decode asynchronously to SW). In the meantime, the Decoder
may or may not insert it into DPB (ref) for an amount of time depending on the
stream, may or may not at unspecified time pass it to display (depending on
ordering in the stream), and client may also have to hold onto it for a while
while Accelerator/HW is processing it (Decoder doesn't know/want to care about
that delay). Decoder may also be passing the surface to Accelerator as reference
for other surfaces, each surface taking a ref to ensure that when decode runs in
hardware the reference surface is still alive. This is because the Decoder may
drop the ref once it doesn't need it anymore for reference/flushes the DPB, but
it may still be used in Accelerator for decode and/or reference and/or display
in client.

[DaleCurtis, 2015-01-14 02:21:08]

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

[DaleCurtis, 2015-01-14 02:21:09]

media/h264 stuff lgtm - didn't review gpu/media

[wuchengli, 2015-01-14 03:07:20]

LGTM

[Pawel Osciak, 2015-01-14 10:35:44]

The CQ bit was checked by posciak@chromium.org

[commit-bot: I haz the power, 2015-01-14 10:36:23]

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

[commit-bot: I haz the power, 2015-01-14 10:37:35]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2015-01-14 10:38:29]

Patchset 10 (id:??) landed as
https://crrev.com/04272d3017214b966feba379d78b195e2fd2193d
Cr-Commit-Position: refs/heads/master@{#311448}

[Pawel Osciak, 2015-01-14 10:53:09]

A revert of this CL (patchset #10 id:220001) has been created in
https://chromiumcodereview.appspot.com/850883002/ by posciak@chromium.org.

The reason for reverting is:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....

[Pawel Osciak, 2015-01-14 11:10:36]

Patchset #11 (id:240001) has been deleted

[scherkus (not reviewing), 2015-01-14 18:44:13]

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
File content/common/gpu/media/h264_dpb.h (right):

https://chromiumcodereview.appspot.com/833063003/diff/140001/content/common/g...
content/common/gpu/media/h264_dpb.h:75: virtual V4L2H264Picture*
AsV4L2H264Picture() { return nullptr; }
On 2015/01/13 11:33:34, Pawel Osciak wrote:
> On 2015/01/13 01:24:59, scherkus wrote:
> > is it ever reasonable to construct an H264Picture as opposed to a subclass?
> 
> Yes, if we intend to use H264Decoder without hiding any implementation details
> in H264Picture.
> 
> > if it isn't, I would make this pure virtual
> > 
> > also .. why not just stick this pure virtual method on H264PictureBase? at
> least
> > as far as I can tell I don't see any other subclasses of H264PictureBase
> 
> The only reason for the ugly existence of H264PictureBase was that I wanted to
> have a memset(this,0,...) constructor for it. I'm not aware of any other
method
> to do this...

That's a poor reason if you ask me :)

You're justifying H264PictureBase and increased complexity throughout the
codebase so you can save a few lines of code inside of the H264PictureBase
constructor by calling memset(this, ...).

This is C++ code. Please set all fields to 0/false in the initializer list and
clean up the class hierarchy.