Build AVC decoder configuration record

Build AVC decoder configuration record

This CL adds a utility to build an AVC decoder configuration record from SPS and PPS data in the H264 stream.

This record is used on Mac to initialize the video decoder.

BUG=127414
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141103

[sail, 2012-05-23 05:32:30]

Hi Ami, this is an alternative to the CL that plumbs the config record to the
decoder.

As you can see there are a couple of places where I have to guess at values when
rebuilding the record.

Since this record is already parsed by both the HTML5 code path and the pepper
flash code path I'd like to avoid rebuilding it.

Let me know what you think.

Thanks,

[Ami GONE FROM CHROMIUM, 2012-05-23 19:41:19]

+posciak to review the use of H264Parser.  Please see my direct question to you
below, and also review the SPS/PPS processing for correctness (as you know I
know little about that stuff).

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
File content/common/gpu/media/avc_config_record_builder.cc (right):

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder.cc:38: can_build_record_ =
true;
The fact that the previous two variables are only ever set to their respective
values together makes me think this is actually a single bit, not two.  And that
makes me think that the API for this class can substitute the three methods:
ProcessNextNALU, BuildConfigRecord, can_build_record with the single method:

bool ProcessNALU(H264Parser* parser, const H264NALU& nalu, 
                 std::vector<uint8_t>* config_record) {
  // Either incrementally build up private state and leave "config_record"
empty, or populate "config_record" indicating to the caller that "nalu" was not
consumed.
}

WDYT?

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder.cc:61: std::vector<uint8_t>
extra_data(record_size, 0);
, 0
unnecessary

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder.cc:73: // TODO(sail): There's
no way to get the NALU field size from the
s/NALU/NALU length/

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder.cc:73: // TODO(sail): There's
no way to get the NALU field size from the
posciak: do you have thoughts about this?

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder.cc:77: // The first 6 bits
are reserved and  must be 1. Last two bits are the
s/and  must/and must/

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder.cc:96: // The number of PPS
units.
Seems like l.83-94 & l.97-107 are near-copies of each other.  De-dup?

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
File content/common/gpu/media/avc_config_record_builder.h (right):

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder.h:10: #include
"base/memory/ref_counted.h"
unnecessary?

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
File content/common/gpu/media/avc_config_record_builder_unittest.cc (right):

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder_unittest.cc:12: const uint8_t
SPS_DATA[] = {
kSPSData (here and below).

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder_unittest.cc:18: const
uint8_t* SPS_DATA_END = SPS_DATA + sizeof(SPS_DATA);
Instead of the _END's, use arraysize() at the use-sites?

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder_unittest.cc:34:
stream.insert(stream.end(), NALU_HEADER, NALU_HEADER_END);
s/.insert/.append/ here and below?

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder_unittest.cc:76:
config_data.begin(), config_data.end(), EXPECTED_DATA));
If you replaced this with:
std::vector<uint8_t> expected_config_data(EXPECTED_DATA, EXPECTED_DATA +
arraysize(EXPECTED_DATA));
EXPECT_EQ(config_data, expected_config_data);

Then failure messages would be more informative.

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/ma...
File content/common/gpu/media/mac_video_decode_accelerator.h (right):

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/ma...
content/common/gpu/media/mac_video_decode_accelerator.h:68: void
DecodeNALU(const content::H264NALU* nalu,
const* is non-idiomatic in chromium (or google) style.
Switch to const&.

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/ma...
File content/common/gpu/media/mac_video_decode_accelerator.mm (right):

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/ma...
content/common/gpu/media/mac_video_decode_accelerator.mm:109:
h264_parser_.SetStream(static_cast<const uint8_t*>(memory.memory()),
The fact that you feed all bitstreambuffers to the parser (not just until
did_build_config_record_ becomes true) makes me think you want to support
mid-stream config changes.
But the code below doesn't do that.
ISTM you either want to add config-change detection, or give up on that and stop
feeding NALUs to the parser once you build a config record.

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/ma...
content/common/gpu/media/mac_video_decode_accelerator.mm:264: // Assume the NALU
lenght field size is 4 bytes.
typo: length

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/ma...
content/common/gpu/media/mac_video_decode_accelerator.mm:266:
std::vector<uint8_t> data(kNALULengthFieldSize + nalu->size, 0);
Why the ", 0"?

[sail, 2012-05-28 21:45:46]

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
File content/common/gpu/media/avc_config_record_builder.cc (right):

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder.cc:38: can_build_record_ =
true;
On 2012/05/23 19:41:19, Ami Fischman wrote:
> The fact that the previous two variables are only ever set to their respective
> values together makes me think this is actually a single bit, not two.  And
that
> makes me think that the API for this class can substitute the three methods:
> ProcessNextNALU, BuildConfigRecord, can_build_record with the single method:
> 
> bool ProcessNALU(H264Parser* parser, const H264NALU& nalu, 
>                  std::vector<uint8_t>* config_record) {
>   // Either incrementally build up private state and leave "config_record"
> empty, or populate "config_record" indicating to the caller that "nalu" was
not
> consumed.
> }
> 
> WDYT?

Done.

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder.cc:61: std::vector<uint8_t>
extra_data(record_size, 0);
On 2012/05/23 19:41:19, Ami Fischman wrote:
> , 0
> unnecessary

Done.

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder.cc:73: // TODO(sail): There's
no way to get the NALU field size from the
On 2012/05/23 19:41:19, Ami Fischman wrote:
> s/NALU/NALU length/

Done.

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder.cc:77: // The first 6 bits
are reserved and  must be 1. Last two bits are the
On 2012/05/23 19:41:19, Ami Fischman wrote:
> s/and  must/and must/

Done.

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder.cc:96: // The number of PPS
units.
On 2012/05/23 19:41:19, Ami Fischman wrote:
> Seems like l.83-94 & l.97-107 are near-copies of each other.  De-dup?

Done.

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
File content/common/gpu/media/avc_config_record_builder.h (right):

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder.h:10: #include
"base/memory/ref_counted.h"
On 2012/05/23 19:41:19, Ami Fischman wrote:
> unnecessary?

used for scoped_refptr

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
File content/common/gpu/media/avc_config_record_builder_unittest.cc (right):

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder_unittest.cc:12: const uint8_t
SPS_DATA[] = {
On 2012/05/23 19:41:19, Ami Fischman wrote:
> kSPSData (here and below).

Done.

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder_unittest.cc:18: const
uint8_t* SPS_DATA_END = SPS_DATA + sizeof(SPS_DATA);
On 2012/05/23 19:41:19, Ami Fischman wrote:
> Instead of the _END's, use arraysize() at the use-sites?

Done.

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder_unittest.cc:18: const
uint8_t* SPS_DATA_END = SPS_DATA + sizeof(SPS_DATA);
On 2012/05/23 19:41:19, Ami Fischman wrote:
> Instead of the _END's, use arraysize() at the use-sites?

Done.

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder_unittest.cc:34:
stream.insert(stream.end(), NALU_HEADER, NALU_HEADER_END);
On 2012/05/23 19:41:19, Ami Fischman wrote:
> s/.insert/.append/ here and below?

hm... vector doesn't seem to have an append method

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder_unittest.cc:76:
config_data.begin(), config_data.end(), EXPECTED_DATA));
On 2012/05/23 19:41:19, Ami Fischman wrote:
> If you replaced this with:
> std::vector<uint8_t> expected_config_data(EXPECTED_DATA, EXPECTED_DATA +
> arraysize(EXPECTED_DATA));
> EXPECT_EQ(config_data, expected_config_data);
> 
> Then failure messages would be more informative.

Done.

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/ma...
File content/common/gpu/media/mac_video_decode_accelerator.h (right):

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/ma...
content/common/gpu/media/mac_video_decode_accelerator.h:68: void
DecodeNALU(const content::H264NALU* nalu,
On 2012/05/23 19:41:19, Ami Fischman wrote:
> const* is non-idiomatic in chromium (or google) style.
> Switch to const&.

Done.

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/ma...
File content/common/gpu/media/mac_video_decode_accelerator.mm (right):

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/ma...
content/common/gpu/media/mac_video_decode_accelerator.mm:109:
h264_parser_.SetStream(static_cast<const uint8_t*>(memory.memory()),
On 2012/05/23 19:41:19, Ami Fischman wrote:
> The fact that you feed all bitstreambuffers to the parser (not just until
> did_build_config_record_ becomes true) makes me think you want to support
> mid-stream config changes.
> But the code below doesn't do that.
> ISTM you either want to add config-change detection, or give up on that and
stop
> feeding NALUs to the parser once you build a config record.

I'd like to keep using the parser so that I can handle multiple NALUs in a
single call to Decode().

Not handling config changes shouldn't affect it.

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/ma...
content/common/gpu/media/mac_video_decode_accelerator.mm:264: // Assume the NALU
lenght field size is 4 bytes.
On 2012/05/23 19:41:19, Ami Fischman wrote:
> typo: length

Done.

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/ma...
content/common/gpu/media/mac_video_decode_accelerator.mm:266:
std::vector<uint8_t> data(kNALULengthFieldSize + nalu->size, 0);
On 2012/05/23 19:41:19, Ami Fischman wrote:
> Why the ", 0"?

Done.

[Ami GONE FROM CHROMIUM, 2012-05-30 00:07:54]

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
File content/common/gpu/media/avc_config_record_builder.cc (right):

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
content/common/gpu/media/avc_config_record_builder.cc:37: // Skip this NALU.
Comment is outdated?

(what is the correct thing to do if we see a non-SPS/PPS/slice NALU?)

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
content/common/gpu/media/avc_config_record_builder.cc:79: if
(!sps_nalus_.empty())
CNTCR() is safe to call w/ an empty vector, so this conditional is unnecessary?

(ditto for PPS below)

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
content/common/gpu/media/avc_config_record_builder.cc:95: // High byte of the
SPS unit size.
drop SPS here and below.

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
File content/common/gpu/media/avc_config_record_builder.h (right):

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
content/common/gpu/media/avc_config_record_builder.h:43: // Copyies data from
|nalus| into |record_buffer|. Returns the number of bytes
s/Copyies/Copies/

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
File content/common/gpu/media/avc_config_record_builder_unittest.cc (right):

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
content/common/gpu/media/avc_config_record_builder_unittest.cc:59: const uint8_t
EXPECTED_DATA[] = {
kExpectedData

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
File content/common/gpu/media/mac_video_decode_accelerator.mm (right):

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:134:
client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id());
Isn't this (prematurely) duplicating the NEOBB notification?

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:137:
RETURN_ON_FAILURE(result = content::H264Parser::kOk,
Assignment instead of comparison.

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:140: bool
did_consume_nalu = false;
unused

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:152: continue;
unreachable

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:167: "Call to
AssignPictureBuffers() during invalid state.",
This pattern generally is incorrect because of queuing.
For example if the client queues two IPCs and the first errors out, the
component will be in an error state when the second is executed, even though the
client couldn't have retracted it by the time the error was reported.

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:193:
RETURN_ON_FAILURE(client_,
did you mean to s/client_/vda_support_/ (here and elsewhere)?

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:242: // TODO(sail):
this assumes Decode() is handed a single NALU at a time. Make
Isn't this assumption tripped up by the vda_unittest case that feeds multiple
NALUs at once?

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:279: DCHECK(client_);
client_ might be gone if a parse error occurs before the AVC config has been
found.

[Pawel Osciak, 2012-05-30 00:28:39]

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
File content/common/gpu/media/avc_config_record_builder.cc (right):

http://codereview.chromium.org/10411085/diff/4001/content/common/gpu/media/av...
content/common/gpu/media/avc_config_record_builder.cc:73: // TODO(sail): There's
no way to get the NALU field size from the
On 2012/05/23 19:41:19, Ami Fischman wrote:
> posciak: do you have thoughts about this?

From what I discussed with Sailesh and saw in the spec (not the H264 spec
though, this is not covered by it), this is field size of a field that is in the
container, not the H264 stream itself. H264Parser operates on stream that has
that data stripped already, so it can't provide this info.

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/h...
File content/common/gpu/media/h264_parser.cc (right):

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/h...
content/common/gpu/media/h264_parser.cc:657: READ_BITS_OR_RETURN(8,
&sps->constraint_setx_flag);
There are only 6 bits of flags, the last 2 bits are reserved. I think this
should be 6 bit and you should add an additional read of reserved 2 bits. Just
to prevent possible confusion in the future.

[sail, 2012-06-02 21:05:49]

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
File content/common/gpu/media/avc_config_record_builder.cc (right):

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
content/common/gpu/media/avc_config_record_builder.cc:37: // Skip this NALU.
On 2012/05/30 00:07:54, Ami Fischman wrote:
> Comment is outdated?

Returning true effectively skips the NALU. Updated the commented to make this
more clear.

> (what is the correct thing to do if we see a non-SPS/PPS/slice NALU?)

The builder must skip these NALUs otherwise it won't be able to build a config
record. For example, the steam in test-25fps.h264 starts with a SEI Message
type.

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
content/common/gpu/media/avc_config_record_builder.cc:79: if
(!sps_nalus_.empty())
On 2012/05/30 00:07:54, Ami Fischman wrote:
> CNTCR() is safe to call w/ an empty vector, so this conditional is
unnecessary?
> 
> (ditto for PPS below)

Done.

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
content/common/gpu/media/avc_config_record_builder.cc:95: // High byte of the
SPS unit size.
On 2012/05/30 00:07:54, Ami Fischman wrote:
> drop SPS here and below.

Done.

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
File content/common/gpu/media/avc_config_record_builder.h (right):

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
content/common/gpu/media/avc_config_record_builder.h:43: // Copyies data from
|nalus| into |record_buffer|. Returns the number of bytes
On 2012/05/30 00:07:54, Ami Fischman wrote:
> s/Copyies/Copies/

Done.

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
File content/common/gpu/media/avc_config_record_builder_unittest.cc (right):

http://codereview.chromium.org/10411085/diff/14003/content/common/gpu/media/a...
content/common/gpu/media/avc_config_record_builder_unittest.cc:59: const uint8_t
EXPECTED_DATA[] = {
On 2012/05/30 00:07:54, Ami Fischman wrote:
> kExpectedData

Done.

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/h...
File content/common/gpu/media/h264_parser.cc (right):

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/h...
content/common/gpu/media/h264_parser.cc:657: READ_BITS_OR_RETURN(8,
&sps->constraint_setx_flag);
On 2012/05/30 00:28:40, posciak wrote:
> There are only 6 bits of flags, the last 2 bits are reserved. I think this
> should be 6 bit and you should add an additional read of reserved 2 bits. Just
> to prevent possible confusion in the future.

Done.
It looks like the last 4 bytes are supposed to be reserved.

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
File content/common/gpu/media/mac_video_decode_accelerator.mm (right):

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:134:
client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id());
On 2012/05/30 00:07:55, Ami Fischman wrote:
> Isn't this (prematurely) duplicating the NEOBB notification?

Yea, this code doesn't handle multiple NALUs in a single call to Decode(). I'll
fix this as a part of bug 130355.

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:137:
RETURN_ON_FAILURE(result = content::H264Parser::kOk,
On 2012/05/30 00:07:55, Ami Fischman wrote:
> Assignment instead of comparison.

Done.

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:140: bool
did_consume_nalu = false;
On 2012/05/30 00:07:55, Ami Fischman wrote:
> unused

Done.

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:152: continue;
On 2012/05/30 00:07:55, Ami Fischman wrote:
> unreachable

Done.

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:193:
RETURN_ON_FAILURE(client_,
On 2012/05/30 00:07:55, Ami Fischman wrote:
> did you mean to s/client_/vda_support_/ (here and elsewhere)?

Done.

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:242: // TODO(sail):
this assumes Decode() is handed a single NALU at a time. Make
On 2012/05/30 00:07:55, Ami Fischman wrote:
> Isn't this assumption tripped up by the vda_unittest case that feeds multiple
> NALUs at once?

Yea, currently the tests that use multiple NALUs at once don't pass.

http://codereview.chromium.org/10411085/diff/24002/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:279: DCHECK(client_);
On 2012/05/30 00:07:55, Ami Fischman wrote:
> client_ might be gone if a parse error occurs before the AVC config has been
> found.

The code only sets client_ to NULL using the RETURN_ON_FAILURE macro. This
should mean that we should never make it this far if a parse error occurred.

[Pawel Osciak, 2012-06-03 17:33:22]

http://codereview.chromium.org/10411085/diff/29001/content/common/gpu/media/h...
File content/common/gpu/media/h264_parser.cc (right):

http://codereview.chromium.org/10411085/diff/29001/content/common/gpu/media/h...
content/common/gpu/media/h264_parser.cc:661: READ_BITS_OR_RETURN(4, &data);
In my version of the spec it's 6 bits of flags, not 4. I'm using the 03/2010
version...

[sail, 2012-06-04 14:57:02]

http://codereview.chromium.org/10411085/diff/29001/content/common/gpu/media/h...
File content/common/gpu/media/h264_parser.cc (right):

http://codereview.chromium.org/10411085/diff/29001/content/common/gpu/media/h...
content/common/gpu/media/h264_parser.cc:661: READ_BITS_OR_RETURN(4, &data);
On 2012/06/03 17:33:22, posciak wrote:
> In my version of the spec it's 6 bits of flags, not 4. I'm using the 03/2010
> version...

Done.

[Ami GONE FROM CHROMIUM, 2012-06-05 15:47:43]

http://codereview.chromium.org/10411085/diff/38001/content/common/gpu/media/m...
File content/common/gpu/media/mac_video_decode_accelerator.mm (right):

http://codereview.chromium.org/10411085/diff/38001/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:166: "Call to
AssignPictureBuffers() during invalid state.",
I think you missed this comment I made on a previous review: 

This pattern generally is incorrect because of queuing.
For example if the client queues two IPCs and the first errors out, the
component will be in an error state when the second is executed, even though the
client couldn't have retracted it by the time the error was reported.

http://codereview.chromium.org/10411085/diff/38001/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:192:
RETURN_ON_FAILURE(client_,
On 2012/06/02 21:05:49, sail wrote:
> On 2012/05/30 00:07:55, Ami Fischman wrote:
> > did you mean to s/client_/vda_support_/ (here and elsewhere)?
> 
> Done.

I don't think it is.

http://codereview.chromium.org/10411085/diff/38001/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:242: // that assumption
go away.
Can you refer to the specific test that doesn't pass, here?

[Ami GONE FROM CHROMIUM, 2012-06-05 16:10:18]

http://codereview.chromium.org/10411085/diff/38001/content/common/gpu/media/m...
File content/common/gpu/media/mac_video_decode_accelerator.mm (right):

http://codereview.chromium.org/10411085/diff/38001/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:166: "Call to
AssignPictureBuffers() during invalid state.",
On 2012/06/05 15:47:43, Ami Fischman wrote:
> I think you missed this comment I made on a previous review: 
> 
> This pattern generally is incorrect because of queuing.
> For example if the client queues two IPCs and the first errors out, the
> component will be in an error state when the second is executed, even though
the
> client couldn't have retracted it by the time the error was reported.

Oops; I was reading this as a DCHECK, not a R_O_F.  Ignore this comment.

[sail, 2012-06-05 16:42:53]

http://codereview.chromium.org/10411085/diff/38001/content/common/gpu/media/m...
File content/common/gpu/media/mac_video_decode_accelerator.mm (right):

http://codereview.chromium.org/10411085/diff/38001/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:192:
RETURN_ON_FAILURE(client_,
On 2012/06/05 15:47:43, Ami Fischman wrote:
> On 2012/06/02 21:05:49, sail wrote:
> > On 2012/05/30 00:07:55, Ami Fischman wrote:
> > > did you mean to s/client_/vda_support_/ (here and elsewhere)?
> > 
> > Done.
> 
> I don't think it is.

Fixed. I changed Flush() and Reset() to check for vda_support_.

http://codereview.chromium.org/10411085/diff/38001/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:242: // that assumption
go away.
On 2012/06/05 15:47:43, Ami Fischman wrote:
> Can you refer to the specific test that doesn't pass, here?

Done.

[Ami GONE FROM CHROMIUM, 2012-06-05 16:48:03]

lgtm

http://codereview.chromium.org/10411085/diff/51004/content/common/gpu/media/m...
File content/common/gpu/media/mac_video_decode_accelerator.mm (right):

http://codereview.chromium.org/10411085/diff/51004/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:192:
RETURN_ON_FAILURE(vda_support_.get(),
.get() unnecessary here and below.

[sail, 2012-06-05 16:57:11]

http://codereview.chromium.org/10411085/diff/51004/content/common/gpu/media/m...
File content/common/gpu/media/mac_video_decode_accelerator.mm (right):

http://codereview.chromium.org/10411085/diff/51004/content/common/gpu/media/m...
content/common/gpu/media/mac_video_decode_accelerator.mm:192:
RETURN_ON_FAILURE(vda_support_.get(),
On 2012/06/05 16:48:03, Ami Fischman wrote:
> .get() unnecessary here and below.

Done.

[sail, 2012-06-05 16:58:46]

brettw: src/content/*.gyp*: OWNERS approval

[Pawel Osciak, 2012-06-05 17:05:14]

LGTM for parser changes.

[brettw, 2012-06-05 19:29:13]

owners lgtm, I didn't check the details

[commit-bot: I haz the power, 2012-06-05 20:13:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10411085/56013

[commit-bot: I haz the power, 2012-06-05 20:13:23]

Failed to apply patch for content/content_tests.gypi:
While running patch -p1 --forward --force;
patching file content/content_tests.gypi
Hunk #1 FAILED at 284.
1 out of 1 hunk FAILED -- saving rejects to file content/content_tests.gypi.rej

[Pawel Osciak, 2012-06-06 20:18:23]

LGTM on win compile fixes for parser.

[commit-bot: I haz the power, 2012-06-07 00:41:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10411085/51009

[commit-bot: I haz the power, 2012-06-07 01:01:41]

Try job failure for 10411085-51009 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-06-07 03:53:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10411085/51009

[commit-bot: I haz the power, 2012-06-07 05:25:02]

Try job failure for 10411085-51009 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-06-07 21:44:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10411085/57010

[commit-bot: I haz the power, 2012-06-07 22:45:57]

Change committed as 141103