Add accelerated VP9 decode infrastructure and an implementation for VA-API.

Add accelerated VP9 decode infrastructure and an implementation for VA-API.

- Add a hardware/platform-independent VP9Decoder class and related
infrastructure, implementing AcceleratedVideoDecoder interface. VP9Decoder
performs the initial stages of the decode process, which are to be done
on host/in software, such as stream parsing and reference frame management.

- Add a VP9Accelerator interface, used by the VP9Decoder to offload the
remaining stages of the decode process to hardware. VP9Accelerator
implementations are platform-specific.

- Add the first implementation of VP9Accelerator - VaapiVP9Accelerator - and
integrate it with VaapiVideoDecodeAccelerator, for devices which provide
hardware VP9 acceleration through VA-API. Hook it up to the new
infrastructure and VP9Decoder.

- Extend Vp9Parser to provide functionality required by VP9Decoder and
VP9Accelerator, including superframe parsing, handling of loop filter
and segmentation initialization, state persistence across frames and
resetting when needed. Also add code calculating segmentation dequants
and loop filter levels.

- Update vp9_parser_unittest to the new Vp9Parser interface and flow.

TEST=vp9_parser_unittest,vda_unittest,Chrome VP9 playback
BUG=chrome-os-partner:41469, chrome-os-partner:41470, chromium:525331
TBR=dpranke@chromium.org

Committed: https://crrev.com/e3cc0a661b8abfdc74f569940949bc1f336ece40
Cr-Commit-Position: refs/heads/master@{#349312}

[Pawel Osciak, 2015-08-31 04:58:08]

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

[Pawel Osciak, 2015-08-31 05:00:05]

ptal

[kcwu, 2015-08-31 09:58:35]

(reviewed parser part only)

https://codereview.chromium.org/1318863003/diff/1/media/filters/vp9_parser.cc
File media/filters/vp9_parser.cc (right):

https://codereview.chromium.org/1318863003/diff/1/media/filters/vp9_parser.cc...
media/filters/vp9_parser.cc:343: fhdr->refresh_flags = reader_.ReadLiteral(8);
s/8/kVp9NumRefFrames/

https://codereview.chromium.org/1318863003/diff/1/media/filters/vp9_parser.cc...
media/filters/vp9_parser.cc:348: fhdr->refresh_flags = reader_.ReadLiteral(8);
s/8/kVp9NumRefFrames/

https://codereview.chromium.org/1318863003/diff/1/media/filters/vp9_parser.cc...
media/filters/vp9_parser.cc:396: SetupSegmentationDequant(fhdr->quant_params);
Does these depend on UpdateSlots() ? If not, I'd prefer UpdateSlots() after
everything else.

https://codereview.chromium.org/1318863003/diff/1/media/filters/vp9_parser.cc...
media/filters/vp9_parser.cc:496: return true;
should we check bytes_left == 0?

https://codereview.chromium.org/1318863003/diff/1/media/filters/vp9_parser.h
File media/filters/vp9_parser.h (right):

https://codereview.chromium.org/1318863003/diff/1/media/filters/vp9_parser.h#...
media/filters/vp9_parser.h:44: EIGHTTAP = 0,
ah, sorry for the bug

https://codereview.chromium.org/1318863003/diff/1/media/filters/vp9_parser.h#...
media/filters/vp9_parser.h:132: bool RefreshFlag(size_t i) const { return
refresh_flags & (1u << i); }
|reader_| is big endian and MSB first inside integer, but you index it using LSB
first. Is it correct?

That's why I stored them one by one into array.

[kcwu, 2015-09-02 10:09:45]

https://codereview.chromium.org/1318863003/diff/1/content/common/gpu/media/va...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1318863003/diff/1/content/common/gpu/media/va...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:1597:
memset(&pic_param, 0, sizeof(VADecPictureParameterBufferVP9));
sizeof(pic_param)

https://codereview.chromium.org/1318863003/diff/1/content/common/gpu/media/vp...
File content/common/gpu/media/vp9_decoder.cc (right):

https://codereview.chromium.org/1318863003/diff/1/content/common/gpu/media/vp...
content/common/gpu/media/vp9_decoder.cc:62: return kDecodeError;
Should we update |state_| as well for all errors?

https://codereview.chromium.org/1318863003/diff/1/content/common/gpu/media/vp...
content/common/gpu/media/vp9_decoder.cc:77: }
continue; here?

https://codereview.chromium.org/1318863003/diff/1/content/common/gpu/media/vp...
content/common/gpu/media/vp9_decoder.cc:81: if (new_pic_size.IsEmpty())
How about DCHECK?
IIUC, the parser never return frames with zero dimension.

https://codereview.chromium.org/1318863003/diff/1/content/common/gpu/media/vp...
content/common/gpu/media/vp9_decoder.cc:95: } else {
Theoretically the parser may return frames with resolution change for
non-keyframe. Should we handle them or at least give warning?

https://codereview.chromium.org/1318863003/diff/1/content/common/gpu/media/vp...
content/common/gpu/media/vp9_decoder.cc:116: if (pic->frame_hdr->IsKeyframe() ||
pic->frame_hdr->RefreshFlag(i))
RefreshFlag should be enough.
How about
DCHECK_IMPLIES(pic->frame_hdr->IsKeyframe(), pic->frame_hdr->RefreshFlag(i));

https://codereview.chromium.org/1318863003/diff/1/content/common/gpu/media/vp...
content/common/gpu/media/vp9_decoder.cc:127: if (frame_hdr->width == 0 ||
frame_hdr->height == 0) {
DCHECK instead?

https://codereview.chromium.org/1318863003/diff/1/content/common/gpu/media/vp...
content/common/gpu/media/vp9_decoder.cc:151: return media::kVp9NumRefFrames +
kPicsInPipeline;
I don't understand these numbers from variable names. Could you give brief
comments? And what is 2?

https://codereview.chromium.org/1318863003/diff/1/content/common/gpu/media/vp...
File content/common/gpu/media/vp9_decoder.h (right):

https://codereview.chromium.org/1318863003/diff/1/content/common/gpu/media/vp...
content/common/gpu/media/vp9_decoder.h:16: // This class provides is and
AcceleratedVideoDecoder for VP9 decoding.
I don't understand this sentence. s/is and/and is/ ?

https://codereview.chromium.org/1318863003/diff/1/content/common/gpu/media/vp...
content/common/gpu/media/vp9_decoder.h:99: kError,               // Error in
decode, can't continue.
Not used?

https://codereview.chromium.org/1318863003/diff/1/media/filters/vp9_parser.cc
File media/filters/vp9_parser.cc (right):

https://codereview.chromium.org/1318863003/diff/1/media/filters/vp9_parser.cc...
media/filters/vp9_parser.cc:33: const int kMaxLoopFilter = 63;
put before functions

[Pawel Osciak, 2015-09-08 06:43:49]

https://chromiumcodereview.appspot.com/1318863003/diff/1/content/common/gpu/m...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1318863003/diff/1/content/common/gpu/m...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:1597:
memset(&pic_param, 0, sizeof(VADecPictureParameterBufferVP9));
On 2015/09/02 10:09:44, kcwu wrote:
> sizeof(pic_param)

Ah, good catch. Done.

https://chromiumcodereview.appspot.com/1318863003/diff/1/content/common/gpu/m...
File content/common/gpu/media/vp9_decoder.cc (right):

https://chromiumcodereview.appspot.com/1318863003/diff/1/content/common/gpu/m...
content/common/gpu/media/vp9_decoder.cc:62: return kDecodeError;
On 2015/09/02 10:09:45, kcwu wrote:
> Should we update |state_| as well for all errors?

Yes, was missing some cleanup.

https://chromiumcodereview.appspot.com/1318863003/diff/1/content/common/gpu/m...
content/common/gpu/media/vp9_decoder.cc:77: }
On 2015/09/02 10:09:45, kcwu wrote:
> continue; here?

I treat this as a critical error.

https://chromiumcodereview.appspot.com/1318863003/diff/1/content/common/gpu/m...
content/common/gpu/media/vp9_decoder.cc:81: if (new_pic_size.IsEmpty())
On 2015/09/02 10:09:44, kcwu wrote:
> How about DCHECK?
> IIUC, the parser never return frames with zero dimension.

Misread the code, true, this is not needed.

https://chromiumcodereview.appspot.com/1318863003/diff/1/content/common/gpu/m...
content/common/gpu/media/vp9_decoder.cc:95: } else {
On 2015/09/02 10:09:45, kcwu wrote:
> Theoretically the parser may return frames with resolution change for
> non-keyframe. Should we handle them or at least give warning?

Good point, this is new in VP9.

https://chromiumcodereview.appspot.com/1318863003/diff/1/content/common/gpu/m...
content/common/gpu/media/vp9_decoder.cc:116: if (pic->frame_hdr->IsKeyframe() ||
pic->frame_hdr->RefreshFlag(i))
On 2015/09/02 10:09:45, kcwu wrote:
> RefreshFlag should be enough.
> How about
> DCHECK_IMPLIES(pic->frame_hdr->IsKeyframe(), pic->frame_hdr->RefreshFlag(i));

Correct, I had moved changing refresh flags to another location, but then
decided to keep it, but didn't change this. Thanks for the idea with
DCHECK_IMPLIES.

https://chromiumcodereview.appspot.com/1318863003/diff/1/content/common/gpu/m...
content/common/gpu/media/vp9_decoder.cc:127: if (frame_hdr->width == 0 ||
frame_hdr->height == 0) {
On 2015/09/02 10:09:45, kcwu wrote:
> DCHECK instead?

Acknowledged.

https://chromiumcodereview.appspot.com/1318863003/diff/1/content/common/gpu/m...
content/common/gpu/media/vp9_decoder.cc:151: return media::kVp9NumRefFrames +
kPicsInPipeline;
On 2015/09/02 10:09:44, kcwu wrote:
> I don't understand these numbers from variable names. Could you give brief
> comments? And what is 2?

We should ideally have the media::limits::kMaxVideoFrames term in higher levels,
but it's not done right now. I'll change this a bit.

https://chromiumcodereview.appspot.com/1318863003/diff/1/content/common/gpu/m...
File content/common/gpu/media/vp9_decoder.h (right):

https://chromiumcodereview.appspot.com/1318863003/diff/1/content/common/gpu/m...
content/common/gpu/media/vp9_decoder.h:16: // This class provides is and
AcceleratedVideoDecoder for VP9 decoding.
On 2015/09/02 10:09:45, kcwu wrote:
> I don't understand this sentence. s/is and/and is/ ?

Done.

https://chromiumcodereview.appspot.com/1318863003/diff/1/content/common/gpu/m...
content/common/gpu/media/vp9_decoder.h:99: kError,               // Error in
decode, can't continue.
On 2015/09/02 10:09:45, kcwu wrote:
> Not used?

Done.

https://chromiumcodereview.appspot.com/1318863003/diff/1/media/filters/vp9_pa...
File media/filters/vp9_parser.cc (right):

https://chromiumcodereview.appspot.com/1318863003/diff/1/media/filters/vp9_pa...
media/filters/vp9_parser.cc:33: const int kMaxLoopFilter = 63;
On 2015/09/02 10:09:45, kcwu wrote:
> put before functions

Hmm, I considered that, but didn't want to have two anonymous namespaces in the
file.

https://chromiumcodereview.appspot.com/1318863003/diff/1/media/filters/vp9_pa...
media/filters/vp9_parser.cc:343: fhdr->refresh_flags = reader_.ReadLiteral(8);
On 2015/08/31 09:58:34, kcwu wrote:
> s/8/kVp9NumRefFrames/

I intentionally used 8 explicitly to keep the convention of using values as per
spec. If refresh_flags was an array, I'd have used kVp9NumRefFrames (or rather,
arraysize).

https://chromiumcodereview.appspot.com/1318863003/diff/1/media/filters/vp9_pa...
media/filters/vp9_parser.cc:396: SetupSegmentationDequant(fhdr->quant_params);
On 2015/08/31 09:58:34, kcwu wrote:
> Does these depend on UpdateSlots() ? If not, I'd prefer UpdateSlots() after
> everything else.

Done.

https://chromiumcodereview.appspot.com/1318863003/diff/1/media/filters/vp9_pa...
media/filters/vp9_parser.cc:496: return true;
On 2015/08/31 09:58:34, kcwu wrote:
> should we check bytes_left == 0?

We could, but I intentionally allow it, instead of failing to decode. It's not
specified whether all bytes before the index have to be used for frame data, and
if some bytes were left unused, it should not be an issue, apart from possibly
broken/corrupted stream.

https://chromiumcodereview.appspot.com/1318863003/diff/1/media/filters/vp9_pa...
File media/filters/vp9_parser.h (right):

https://chromiumcodereview.appspot.com/1318863003/diff/1/media/filters/vp9_pa...
media/filters/vp9_parser.h:44: EIGHTTAP = 0,
On 2015/08/31 09:58:35, kcwu wrote:
> ah, sorry for the bug

No worries.

https://chromiumcodereview.appspot.com/1318863003/diff/1/media/filters/vp9_pa...
media/filters/vp9_parser.h:132: bool RefreshFlag(size_t i) const { return
refresh_flags & (1u << i); }
On 2015/08/31 09:58:35, kcwu wrote:
> |reader_| is big endian and MSB first inside integer, but you index it using
LSB
> first. Is it correct?
> 
> That's why I stored them one by one into array.

Yes, I was surprised too, but spec and libvpx both read L(8) at once and process
from LSB.

[kcwu, 2015-09-08 09:08:41]

https://chromiumcodereview.appspot.com/1318863003/diff/1/media/filters/vp9_pa...
File media/filters/vp9_parser.cc (right):

https://chromiumcodereview.appspot.com/1318863003/diff/1/media/filters/vp9_pa...
media/filters/vp9_parser.cc:33: const int kMaxLoopFilter = 63;
On 2015/09/08 06:43:49, Pawel Osciak wrote:
> On 2015/09/02 10:09:45, kcwu wrote:
> > put before functions
> 
> Hmm, I considered that, but didn't want to have two anonymous namespaces in
the
> file.

Two anonymous namespaces? I mean put it at line 12.

https://chromiumcodereview.appspot.com/1318863003/diff/20001/content/common/g...
File content/common/gpu/media/vp9_decoder.cc (right):

https://chromiumcodereview.appspot.com/1318863003/diff/20001/content/common/g...
content/common/gpu/media/vp9_decoder.cc:64: SetError();
Assume Decode() failed last time, the coder enter kError state. If calling
Decode() again, it will return kRanOutOfStreamData due to parser was reset.

I expect Decode() would return kDecodeError once it failed.

https://chromiumcodereview.appspot.com/1318863003/diff/20001/content/common/g...
content/common/gpu/media/vp9_decoder.cc:95: }
continue; after OutputPicture succeeded. The code flow of show_existing_frame
should not fall through line 97.

https://chromiumcodereview.appspot.com/1318863003/diff/20001/content/common/g...
content/common/gpu/media/vp9_decoder.cc:98: if (new_pic_size.IsEmpty()) {
You said this is not needed but it is still here?
DCHECK instead?

[Pawel Osciak, 2015-09-08 10:49:22]

Patchset #3 (id:40001) has been deleted

[Pawel Osciak, 2015-09-08 10:54:03]

Patchset #3 (id:60001) has been deleted

[Pawel Osciak, 2015-09-08 11:06:37]

Patchset #3 (id:80001) has been deleted

[Pawel Osciak, 2015-09-08 11:08:08]

https://chromiumcodereview.appspot.com/1318863003/diff/1/media/filters/vp9_pa...
File media/filters/vp9_parser.cc (right):

https://chromiumcodereview.appspot.com/1318863003/diff/1/media/filters/vp9_pa...
media/filters/vp9_parser.cc:33: const int kMaxLoopFilter = 63;
On 2015/09/08 09:08:41, kcwu wrote:
> On 2015/09/08 06:43:49, Pawel Osciak wrote:
> > On 2015/09/02 10:09:45, kcwu wrote:
> > > put before functions
> > 
> > Hmm, I considered that, but didn't want to have two anonymous namespaces in
> the
> > file.
> 
> Two anonymous namespaces? I mean put it at line 12.
> 

Ah, misunderstood then. Sure.

https://chromiumcodereview.appspot.com/1318863003/diff/20001/content/common/g...
File content/common/gpu/media/vp9_decoder.cc (right):

https://chromiumcodereview.appspot.com/1318863003/diff/20001/content/common/g...
content/common/gpu/media/vp9_decoder.cc:64: SetError();
On 2015/09/08 09:08:41, kcwu wrote:
> Assume Decode() failed last time, the coder enter kError state. If calling
> Decode() again, it will return kRanOutOfStreamData due to parser was reset.
> 
> I expect Decode() would return kDecodeError once it failed.

We already returned kDecodeError previously and reset ourselves. Although the
client would normally propagate that error and pack up, destroying us, if it
wanted to continue on a new stream, we are technically good to continue. So this
is intended.

https://chromiumcodereview.appspot.com/1318863003/diff/20001/content/common/g...
content/common/gpu/media/vp9_decoder.cc:95: }
On 2015/09/08 09:08:41, kcwu wrote:
> continue; after OutputPicture succeeded. The code flow of show_existing_frame
> should not fall through line 97.

Ah, I actually misunderstood the spec. Thanks!

https://chromiumcodereview.appspot.com/1318863003/diff/20001/content/common/g...
content/common/gpu/media/vp9_decoder.cc:98: if (new_pic_size.IsEmpty()) {
On 2015/09/08 09:08:41, kcwu wrote:
> You said this is not needed but it is still here?
> DCHECK instead?

True. Done.

[Pawel Osciak, 2015-09-09 04:13:38]

Patchset #3 (id:100001) has been deleted

[Pawel Osciak, 2015-09-09 04:15:42]

ptal

[kcwu, 2015-09-09 06:17:13]

https://chromiumcodereview.appspot.com/1318863003/diff/120001/content/common/...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1318863003/diff/120001/content/common/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:238: bool
SubmitDecode(
why is it inconsistent to VaapiVP8Accelerator?
VaapiVP8Accelerator::SubmitDecode get Vp8FrameHeader as parameter, but
VaapiVP9Accelerator get VP9FrameHeader from VP9Picture.

https://chromiumcodereview.appspot.com/1318863003/diff/120001/content/common/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:1596:
VADecPictureParameterBufferVP9 pic_param;
va_dec_vp9.h says it only support 8-bit 420 format.
Check frame_hdr->bit_depth and color_space, then reject others.

BTW, I found libvpx read and discard value of |yuv_range| immediately.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libvpx...
I'm not sure should we check its value or not.

https://chromiumcodereview.appspot.com/1318863003/diff/120001/content/common/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:1602:
pic_param.frame_width = frame_hdr->width;
width and height could be 2^16 according to spec but pic_param.frame_width has
only 16 bits.

LOG and return false if the value equals 2^16?

https://chromiumcodereview.appspot.com/1318863003/diff/120001/content/common/...
File content/common/gpu/media/vp9_decoder.cc (right):

https://chromiumcodereview.appspot.com/1318863003/diff/120001/content/common/...
content/common/gpu/media/vp9_decoder.cc:151: media::Vp9FrameHeader* frame_hdr =
pic->frame_hdr.get();
const

[Pawel Osciak, 2015-09-09 08:55:41]

https://chromiumcodereview.appspot.com/1318863003/diff/120001/content/common/...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1318863003/diff/120001/content/common/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:238: bool
SubmitDecode(
On 2015/09/09 06:17:12, kcwu wrote:
> why is it inconsistent to VaapiVP8Accelerator?
> VaapiVP8Accelerator::SubmitDecode get Vp8FrameHeader as parameter, but
> VaapiVP9Accelerator get VP9FrameHeader from VP9Picture.

This is intentional. I'd like to change VP8 later as well. Just like with
H264Picture, I'd like to keep related info in VPXPicture. Also less parameters.

https://chromiumcodereview.appspot.com/1318863003/diff/120001/content/common/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:1596:
VADecPictureParameterBufferVP9 pic_param;
On 2015/09/09 06:17:12, kcwu wrote:
> va_dec_vp9.h says it only support 8-bit 420 format.
> Check frame_hdr->bit_depth and color_space, then reject others.
> 

Good suggestion. Since parser verifies these, we should be ok with just checking
profile == 0.

> BTW, I found libvpx read and discard value of |yuv_range| immediately.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libvpx...
> I'm not sure should we check its value or not.

We don't really have a way to communicate this to display layers right now
unfortunately.

https://chromiumcodereview.appspot.com/1318863003/diff/120001/content/common/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:1602:
pic_param.frame_width = frame_hdr->width;
On 2015/09/09 06:17:12, kcwu wrote:
> width and height could be 2^16 according to spec but pic_param.frame_width has
> only 16 bits.
> 
> LOG and return false if the value equals 2^16?

Let's do a checked_cast instead.

https://chromiumcodereview.appspot.com/1318863003/diff/120001/content/common/...
File content/common/gpu/media/vp9_decoder.cc (right):

https://chromiumcodereview.appspot.com/1318863003/diff/120001/content/common/...
content/common/gpu/media/vp9_decoder.cc:151: media::Vp9FrameHeader* frame_hdr =
pic->frame_hdr.get();
On 2015/09/09 06:17:12, kcwu wrote:
> const

Ah, this used to be used more than once in earlier versions, but now probably
better to just remove it.

[kcwu, 2015-09-09 09:02:53]

lgtm

https://chromiumcodereview.appspot.com/1318863003/diff/140001/content/common/...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1318863003/diff/140001/content/common/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:1603: DVLOG(1) <<
"Unsupported profile";
<< frame_hdr->profile

[Pawel Osciak, 2015-09-09 10:19:27]

https://chromiumcodereview.appspot.com/1318863003/diff/140001/content/common/...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1318863003/diff/140001/content/common/...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:1603: DVLOG(1) <<
"Unsupported profile";
On 2015/09/09 09:02:53, kcwu wrote:
> << frame_hdr->profile

Done.

[Pawel Osciak, 2015-09-09 10:20:35]

dalecurtis@: please OWNERS for media/. Thanks.

[Pawel Osciak, 2015-09-09 10:25:05]

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

[Pawel Osciak, 2015-09-09 10:25:06]

dalecurtis@: please OWNERS for media/. Thanks.

[Pawel Osciak, 2015-09-15 07:59:06]

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

[Pawel Osciak, 2015-09-15 08:05:56]

sandersd@: please OWNERS for media/.

jorgelo@: please  content/common/sandbox_linux/bpf_gpu_policy_linux.cc.

Thanks!

[DaleCurtis, 2015-09-15 16:27:36]

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

[DaleCurtis, 2015-09-15 16:27:36]

(sorry, I missed this review, feel free to ping me via e-mail next time,
sandersd@ can handle it now though :)

+fgalligan as an FYI since neither dan nor I know the vp9 parsing well.

[Jorge Lucangeli Obes, 2015-09-15 16:54:10]

https://chromiumcodereview.appspot.com/1318863003/diff/180001/content/common/...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://chromiumcodereview.appspot.com/1318863003/diff/180001/content/common/...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:303: const char*
I965HybridDrvVideoPath = NULL;
Technically, "965" is not part of the file name.

[Pawel Osciak, 2015-09-16 00:33:41]

https://chromiumcodereview.appspot.com/1318863003/diff/180001/content/common/...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://chromiumcodereview.appspot.com/1318863003/diff/180001/content/common/...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:303: const char*
I965HybridDrvVideoPath = NULL;
On 2015/09/15 16:54:10, Jorge Lucangeli Obes wrote:
> Technically, "965" is not part of the file name.

The hybrid driver is an integral part of i965 driver, that's why I called it
this way, but I have no preference and I'm happy to change it if you prefer to
drop i965?

[Jorge Lucangeli Obes, 2015-09-16 01:23:49]

On 2015/09/16 00:33:41, Pawel Osciak wrote:
>
https://chromiumcodereview.appspot.com/1318863003/diff/180001/content/common/...
> File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1318863003/diff/180001/content/common/...
> content/common/sandbox_linux/bpf_gpu_policy_linux.cc:303: const char*
> I965HybridDrvVideoPath = NULL;
> On 2015/09/15 16:54:10, Jorge Lucangeli Obes wrote:
> > Technically, "965" is not part of the file name.
> 
> The hybrid driver is an integral part of i965 driver, that's why I called it
> this way, but I have no preference and I'm happy to change it if you prefer to
> drop i965?

Nah, that's fine.

lgtm

[Pawel Osciak, 2015-09-16 12:26:08]

Patchset #7 (id:200001) has been deleted

[Pawel Osciak, 2015-09-16 12:26:26]

Patchset #7 (id:220001) has been deleted

[Pawel Osciak, 2015-09-16 12:26:44]

Patchset #7 (id:240001) has been deleted

[seanvk, 2015-09-16 16:52:54]

On 2015/09/16 01:23:49, Jorge Lucangeli Obes wrote:
> On 2015/09/16 00:33:41, Pawel Osciak wrote:
> >
>
https://chromiumcodereview.appspot.com/1318863003/diff/180001/content/common/...
> > File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/1318863003/diff/180001/content/common/...
> > content/common/sandbox_linux/bpf_gpu_policy_linux.cc:303: const char*
> > I965HybridDrvVideoPath = NULL;
> > On 2015/09/15 16:54:10, Jorge Lucangeli Obes wrote:
> > > Technically, "965" is not part of the file name.
> > 
> > The hybrid driver is an integral part of i965 driver, that's why I called it
> > this way, but I have no preference and I'm happy to change it if you prefer
to
> > drop i965?
> 
> Nah, that's fine.
> 
> lgtm

lgtm

[sandersd (OOO until July 31), 2015-09-17 00:19:53]

RS LGTM for media/filters.

[Pawel Osciak, 2015-09-17 00:20:46]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2015-09-17 00:20:46]

The patchset sent to the CQ was uploaded after l-g-t-m from kcwu@chromium.org,
jorgelo@chromium.org
Link to the patchset: https://codereview.chromium.org/1318863003/#ps260001
(title: " ")

[commit-bot: I haz the power, 2015-09-17 00:22:39]

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

[commit-bot: I haz the power, 2015-09-17 00:56:28]

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

[commit-bot: I haz the power, 2015-09-17 00:56:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Pawel Osciak, 2015-09-17 01:33:32]

TBR=dpranke@ for BUILD.gn please.

[Pawel Osciak, 2015-09-17 01:33:48]

The CQ bit was checked by posciak@chromium.org

[commit-bot: I haz the power, 2015-09-17 01:34:00]

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

[commit-bot: I haz the power, 2015-09-17 01:40:00]

Committed patchset #7 (id:260001)

[commit-bot: I haz the power, 2015-09-17 01:40:38]

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

[tommycli, 2015-09-17 17:35:35]

A revert of this CL (patchset #7 id:260001) has been created in
https://codereview.chromium.org/1357513002/ by tommycli@chromium.org.

The reason for reverting is: I think this patch broke compile step for Chromium
Linux ChromeOS MSan Builder.

First failing build:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Linux%20C...

All recent builds:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Linux%20C...

Sorry for the revert. I'll re-revert if I'm wrong.

Cheers,

Tommy.