Allow multiple texture ids per picture buffer.

Allow multiple texture ids per picture buffer.

DXGI NV12 textures have two planes and each plane should be referenced by its own texture id. Currently the decoder will always ask for 1 texture for picture buffer.

BUG=574292
Committed: https://crrev.com/673b14862f1fc0ffc2eac7c92ee2604f168a479e
Cr-Commit-Position: refs/heads/master@{#384429}

[jbauman, 2016-03-21 23:18:19]

Description was changed from

==========
Allow multiple texture ids per picture buffer.

DXGI NV12 textures have two planes and each plane will be referred to by a
texture id.
==========

to

==========
Allow multiple texture ids per picture buffer.

DXGI NV12 textures have two planes and each plane should be referenced by its
own texture id. Currently the decoder will always ask for 1 texture for picture
buffer.

BUG=574292
==========

[jbauman, 2016-03-21 23:18:33]

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

[DaleCurtis, 2016-03-21 23:26:27]

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

[DaleCurtis, 2016-03-21 23:26:28]

=>sandersd

[sandersd (OOO until July 31), 2016-03-22 01:17:12]

On 2016/03/21 23:26:28, DaleCurtis wrote:
> =>sandersd

I'm withholding final comments because I see that there is still debug code in a
few places. Overall looks good.

It's a bit of an ugly API, in two regards: (1) every PictureBuffer consumer
needs to be aware of this and (2) the size of each texture is the same, even
though most video pixel formats don't have that. It may be possible to fix (2)
with whatever mechanism will pass the pixel format along.

Perhaps (1) can be solved by separating the main plane from extra planes? I'm
not sure what's standard for multi-plane pictures elsewhere, maybe it really is
best to make all consumers aware of planes.

[jbauman, 2016-03-22 01:21:27]

On 2016/03/22 01:17:12, sandersd wrote:
> On 2016/03/21 23:26:28, DaleCurtis wrote:
> > =>sandersd
> 
> I'm withholding final comments because I see that there is still debug code in
a
> few places. Overall looks good.
> 
> It's a bit of an ugly API, in two regards: (1) every PictureBuffer consumer
> needs to be aware of this and (2) the size of each texture is the same, even
> though most video pixel formats don't have that. It may be possible to fix (2)
> with whatever mechanism will pass the pixel format along.

Yeah, I was planning on dealing with that once we have video formats that could
use this.
> 
> Perhaps (1) can be solved by separating the main plane from extra planes? I'm
> not sure what's standard for multi-plane pictures elsewhere, maybe it really
is
> best to make all consumers aware of planes.

There's no way to be ignorant of the planes and actually handle the picture
correctly; you won't have any color information in that case. It's possible that
for some consumers (e.g. pepper) we'd enforce that they also get single-plane
RGBA, while other consumers can get NV12 if supported.

[sandersd (OOO until July 31), 2016-03-22 01:24:14]

On 2016/03/22 01:21:27, jbauman wrote:
> On 2016/03/22 01:17:12, sandersd wrote:
> > On 2016/03/21 23:26:28, DaleCurtis wrote:
> > > =>sandersd
> > 
> > I'm withholding final comments because I see that there is still debug code
in
> a
> > few places. Overall looks good.
> > 
> > It's a bit of an ugly API, in two regards: (1) every PictureBuffer consumer
> > needs to be aware of this and (2) the size of each texture is the same, even
> > though most video pixel formats don't have that. It may be possible to fix
(2)
> > with whatever mechanism will pass the pixel format along.
> 
> Yeah, I was planning on dealing with that once we have video formats that
could
> use this.
> > 
> > Perhaps (1) can be solved by separating the main plane from extra planes?
I'm
> > not sure what's standard for multi-plane pictures elsewhere, maybe it really
> is
> > best to make all consumers aware of planes.
> 
> There's no way to be ignorant of the planes and actually handle the picture
> correctly; you won't have any color information in that case. It's possible
that
> for some consumers (e.g. pepper) we'd enforce that they also get single-plane
> RGBA, while other consumers can get NV12 if supported.

Sorry, I should have specifically said VDAs. Your CL at least shows that the
number of changed lines is small, but it's still questionable since most VDAs
are going to continue to use a single texture.

[jbauman, 2016-03-22 01:28:07]

On 2016/03/22 01:24:14, sandersd wrote:
> On 2016/03/22 01:21:27, jbauman wrote:
> > On 2016/03/22 01:17:12, sandersd wrote:
> > > On 2016/03/21 23:26:28, DaleCurtis wrote:
> > > > =>sandersd
> > > 
> > > I'm withholding final comments because I see that there is still debug
code
> in
> > a
> > > few places. Overall looks good.
> > > 
> > > It's a bit of an ugly API, in two regards: (1) every PictureBuffer
consumer
> > > needs to be aware of this and (2) the size of each texture is the same,
even
> > > though most video pixel formats don't have that. It may be possible to fix
> (2)
> > > with whatever mechanism will pass the pixel format along.
> > 
> > Yeah, I was planning on dealing with that once we have video formats that
> could
> > use this.
> > > 
> > > Perhaps (1) can be solved by separating the main plane from extra planes?
> I'm
> > > not sure what's standard for multi-plane pictures elsewhere, maybe it
really
> > is
> > > best to make all consumers aware of planes.
> > 
> > There's no way to be ignorant of the planes and actually handle the picture
> > correctly; you won't have any color information in that case. It's possible
> that
> > for some consumers (e.g. pepper) we'd enforce that they also get
single-plane
> > RGBA, while other consumers can get NV12 if supported.
> 
> Sorry, I should have specifically said VDAs. Your CL at least shows that the
> number of changed lines is small, but it's still questionable since most VDAs
> are going to continue to use a single texture.

Oh, ok. I think we generally (with regard to media::VideoFrame at least) treat
single-plane pictures as a special case of multiple-plane pictures, so I think
that makes sense here. It's pretty easy to not do anything with the other planes
if you don't support them (and if the producer doesn't try to use them).

[sandersd (OOO until July 31), 2016-03-22 01:29:28]

On 2016/03/22 01:28:07, jbauman wrote:
> On 2016/03/22 01:24:14, sandersd wrote:
> > On 2016/03/22 01:21:27, jbauman wrote:
> > > On 2016/03/22 01:17:12, sandersd wrote:
> > > > On 2016/03/21 23:26:28, DaleCurtis wrote:
> > > > > =>sandersd
> > > > 
> > > > I'm withholding final comments because I see that there is still debug
> code
> > in
> > > a
> > > > few places. Overall looks good.
> > > > 
> > > > It's a bit of an ugly API, in two regards: (1) every PictureBuffer
> consumer
> > > > needs to be aware of this and (2) the size of each texture is the same,
> even
> > > > though most video pixel formats don't have that. It may be possible to
fix
> > (2)
> > > > with whatever mechanism will pass the pixel format along.
> > > 
> > > Yeah, I was planning on dealing with that once we have video formats that
> > could
> > > use this.
> > > > 
> > > > Perhaps (1) can be solved by separating the main plane from extra
planes?
> > I'm
> > > > not sure what's standard for multi-plane pictures elsewhere, maybe it
> really
> > > is
> > > > best to make all consumers aware of planes.
> > > 
> > > There's no way to be ignorant of the planes and actually handle the
picture
> > > correctly; you won't have any color information in that case. It's
possible
> > that
> > > for some consumers (e.g. pepper) we'd enforce that they also get
> single-plane
> > > RGBA, while other consumers can get NV12 if supported.
> > 
> > Sorry, I should have specifically said VDAs. Your CL at least shows that the
> > number of changed lines is small, but it's still questionable since most
VDAs
> > are going to continue to use a single texture.
> 
> Oh, ok. I think we generally (with regard to media::VideoFrame at least) treat
> single-plane pictures as a special case of multiple-plane pictures, so I think
> that makes sense here. It's pretty easy to not do anything with the other
planes
> if you don't support them (and if the producer doesn't try to use them).

Alright, sounds good then. Please send a ping when the remaining code is ready!

[jbauman, 2016-03-22 21:49:52]

On 2016/03/22 01:29:28, sandersd wrote:
> On 2016/03/22 01:28:07, jbauman wrote:
> > On 2016/03/22 01:24:14, sandersd wrote:
> > > On 2016/03/22 01:21:27, jbauman wrote:
> > > > On 2016/03/22 01:17:12, sandersd wrote:
> > > > > On 2016/03/21 23:26:28, DaleCurtis wrote:
> > > > > > =>sandersd
> > > > > 
> > > > > I'm withholding final comments because I see that there is still debug
> > code
> > > in
> > > > a
> > > > > few places. Overall looks good.
> > > > > 
> > > > > It's a bit of an ugly API, in two regards: (1) every PictureBuffer
> > consumer
> > > > > needs to be aware of this and (2) the size of each texture is the
same,
> > even
> > > > > though most video pixel formats don't have that. It may be possible to
> fix
> > > (2)
> > > > > with whatever mechanism will pass the pixel format along.
> > > > 
> > > > Yeah, I was planning on dealing with that once we have video formats
that
> > > could
> > > > use this.
> > > > > 
> > > > > Perhaps (1) can be solved by separating the main plane from extra
> planes?
> > > I'm
> > > > > not sure what's standard for multi-plane pictures elsewhere, maybe it
> > really
> > > > is
> > > > > best to make all consumers aware of planes.
> > > > 
> > > > There's no way to be ignorant of the planes and actually handle the
> picture
> > > > correctly; you won't have any color information in that case. It's
> possible
> > > that
> > > > for some consumers (e.g. pepper) we'd enforce that they also get
> > single-plane
> > > > RGBA, while other consumers can get NV12 if supported.
> > > 
> > > Sorry, I should have specifically said VDAs. Your CL at least shows that
the
> > > number of changed lines is small, but it's still questionable since most
> VDAs
> > > are going to continue to use a single texture.
> > 
> > Oh, ok. I think we generally (with regard to media::VideoFrame at least)
treat
> > single-plane pictures as a special case of multiple-plane pictures, so I
think
> > that makes sense here. It's pretty easy to not do anything with the other
> planes
> > if you don't support them (and if the producer doesn't try to use them).
> 
> Alright, sounds good then. Please send a ping when the remaining code is
ready!

PTAL. Debugging stuff should all be removed.

[sandersd (OOO until July 31), 2016-03-23 00:41:01]

Just 1 small question.

https://codereview.chromium.org/1751323002/diff/160001/media/video/picture.h
File media/video/picture.h (right):

https://codereview.chromium.org/1751323002/diff/160001/media/video/picture.h#...
media/video/picture.h:23: struct TextureIds {
Is there a particular reason to be using a struct, rather than the array
directly?

[jbauman, 2016-03-23 01:01:26]

On 2016/03/23 00:41:01, sandersd wrote:
> Just 1 small question.
> 
> https://codereview.chromium.org/1751323002/diff/160001/media/video/picture.h
> File media/video/picture.h (right):
> 
>
https://codereview.chromium.org/1751323002/diff/160001/media/video/picture.h#...
> media/video/picture.h:23: struct TextureIds {
> Is there a particular reason to be using a struct, rather than the array
> directly?

The ability to use a copy constructor is nice, and then it can also be passed
through Bind or used in a map. I think std::array would work, if you wanted that
(though I'd have to see about how best to pass it through IPC).

[DaleCurtis, 2016-03-23 01:04:02]

Why not just a vector?

[sandersd (OOO until July 31), 2016-03-23 01:04:55]

lgtm

[jbauman, 2016-03-24 01:24:54]

On 2016/03/23 01:04:02, DaleCurtis wrote:
> Why not just a vector?

Yeah, that's a fair point. Modified the patch to use a vector.

[DaleCurtis, 2016-03-24 01:29:06]

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

[DaleCurtis, 2016-03-24 01:29:07]

Is a texture id of zero a valid value?

[jbauman, 2016-03-24 01:31:56]

On 2016/03/24 01:29:07, DaleCurtis wrote:
> Is a texture id of zero a valid value?

No. Why?

[DaleCurtis, 2016-03-24 01:35:37]

Just curious since you have "if (id)" in a lot of places. I defer to sandersd@'s
original review, using the vector lgtm though.

[chromium-reviews, 2016-03-24 01:49:36]

On Wed, Mar 23, 2016 at 6:31 PM, <jbauman@chromium.org> wrote:

> On 2016/03/24 01:29:07, DaleCurtis wrote:
> > Is a texture id of zero a valid value?
>
> No. Why?
>

Deleting 0 is fine though isn't it?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jbauman, 2016-03-24 01:56:07]

On 2016/03/24 01:49:36, chromium-reviews wrote:
> On Wed, Mar 23, 2016 at 6:31 PM, <mailto:jbauman@chromium.org> wrote:
> 
> > On 2016/03/24 01:29:07, DaleCurtis wrote:
> > > Is a texture id of zero a valid value?
> >
> > No. Why?
> >
> 
> Deleting 0 is fine though isn't it?

Yeah. I actually decided to remove those ifs because now all ids in the vector
should be valid (as it's not fixed-length).
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[DaleCurtis, 2016-03-24 02:00:46]

Sorry my comments are coming in sputters :)

https://codereview.chromium.org/1751323002/diff/260001/media/video/picture.cc
File media/video/picture.cc (right):

https://codereview.chromium.org/1751323002/diff/260001/media/video/picture.cc...
media/video/picture.cc:15: TextureIds texture_ids,
const& for both?

https://codereview.chromium.org/1751323002/diff/260001/media/video/picture.cc...
media/video/picture.cc:24: TextureIds texture_ids,
const&?

https://codereview.chromium.org/1751323002/diff/260001/media/video/picture.cc...
media/video/picture.cc:25: gpu::Mailbox texture_mailbox[kMaxPlanes])
Why the const& removal here? Should this be a vector too?

[jbauman, 2016-03-24 19:32:18]

On 2016/03/24 02:00:46, DaleCurtis wrote:
> Sorry my comments are coming in sputters :)
> 
> https://codereview.chromium.org/1751323002/diff/260001/media/video/picture.cc
> File media/video/picture.cc (right):
> 
>
https://codereview.chromium.org/1751323002/diff/260001/media/video/picture.cc...
> media/video/picture.cc:15: TextureIds texture_ids,
> const& for both?

Fixed.
> 
>
https://codereview.chromium.org/1751323002/diff/260001/media/video/picture.cc...
> media/video/picture.cc:24: TextureIds texture_ids,
> const&?

Fixed.
> 
>
https://codereview.chromium.org/1751323002/diff/260001/media/video/picture.cc...
> media/video/picture.cc:25: gpu::Mailbox texture_mailbox[kMaxPlanes])
> Why the const& removal here? Should this be a vector too?

Yeah, changed this to a vector.

[DaleCurtis, 2016-03-24 20:11:24]

types lgtm, thanks!

[jbauman, 2016-03-24 20:55:46]

jbauman@chromium.org changed reviewers:
+ mkwst@chromium.org, raymes@chromium.org

[jbauman, 2016-03-24 20:55:47]

raymes@: content/renderer/pepper OWNERS review
mkwst@: IPC review.

[jbauman, 2016-03-30 02:14:53]

jbauman@chromium.org changed reviewers:
+ kenrb@chromium.org
- mkwst@chromium.org

[jbauman, 2016-03-30 02:14:54]

mkwst@ seems to be OOO, so adding kenrb@ for *_messages* review.

[kenrb, 2016-03-30 13:29:29]

https://codereview.chromium.org/1751323002/diff/280001/content/common/gpu/med...
File content/common/gpu/media/media_messages.h (right):

https://codereview.chromium.org/1751323002/diff/280001/content/common/gpu/med...
content/common/gpu/media/media_messages.h:150: uint32_t,  /* Number of textures
per frame */
Is it possible to enforce a sane maximum value on this? It looks like it becomes
a factor in memory allocation sizes.

[raymes, 2016-03-31 00:09:25]

Sorry I forgot to send my comments a few days ago. 

lgtm

https://codereview.chromium.org/1751323002/diff/280001/content/renderer/peppe...
File content/renderer/pepper/ppb_video_decoder_impl.cc (right):

https://codereview.chromium.org/1751323002/diff/280001/content/renderer/peppe...
content/renderer/pepper/ppb_video_decoder_impl.cc:186:
media::PictureBuffer::TextureIds pepper_buffers;
nit: perhaps keep the naming similar to the other file (pepper_buffers vs ids)

https://codereview.chromium.org/1751323002/diff/280001/content/renderer/peppe...
File content/renderer/pepper/video_decoder_shim.cc (right):

https://codereview.chromium.org/1751323002/diff/280001/content/renderer/peppe...
content/renderer/pepper/video_decoder_shim.cc:948: continue;
Could this case actually happen?

[jbauman, 2016-03-31 02:07:29]

On 2016/03/30 13:29:29, kenrb wrote:
>
https://codereview.chromium.org/1751323002/diff/280001/content/common/gpu/med...
> File content/common/gpu/media/media_messages.h (right):
> 
>
https://codereview.chromium.org/1751323002/diff/280001/content/common/gpu/med...
> content/common/gpu/media/media_messages.h:150: uint32_t,  /* Number of
textures
> per frame */
> Is it possible to enforce a sane maximum value on this? It looks like it
becomes
> a factor in memory allocation sizes.

Ok, added a check that this is <= 4.

[kenrb, 2016-03-31 14:44:40]

ipc lgtm

[jbauman, 2016-03-31 21:40:22]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2016-03-31 21:40:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org, dalecurtis@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/1751323002/#ps360001
(title: " ")

[commit-bot: I haz the power, 2016-03-31 21:40:59]

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

[commit-bot: I haz the power, 2016-03-31 23:21:05]

Description was changed from

==========
Allow multiple texture ids per picture buffer.

DXGI NV12 textures have two planes and each plane should be referenced by its
own texture id. Currently the decoder will always ask for 1 texture for picture
buffer.

BUG=574292
==========

to

==========
Allow multiple texture ids per picture buffer.

DXGI NV12 textures have two planes and each plane should be referenced by its
own texture id. Currently the decoder will always ask for 1 texture for picture
buffer.

BUG=574292
==========

[commit-bot: I haz the power, 2016-03-31 23:21:07]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-03-31 23:22:33]

Description was changed from

==========
Allow multiple texture ids per picture buffer.

DXGI NV12 textures have two planes and each plane should be referenced by its
own texture id. Currently the decoder will always ask for 1 texture for picture
buffer.

BUG=574292
==========

to

==========
Allow multiple texture ids per picture buffer.

DXGI NV12 textures have two planes and each plane should be referenced by its
own texture id. Currently the decoder will always ask for 1 texture for picture
buffer.

BUG=574292

Committed: https://crrev.com/673b14862f1fc0ffc2eac7c92ee2604f168a479e
Cr-Commit-Position: refs/heads/master@{#384429}
==========

[commit-bot: I haz the power, 2016-03-31 23:22:35]

Patchset 19 (id:??) landed as
https://crrev.com/673b14862f1fc0ffc2eac7c92ee2604f168a479e
Cr-Commit-Position: refs/heads/master@{#384429}