Duplicate VideoEncodeAccelerator::SupportedProfile in gpu_info.h.

Duplicate VideoEncodeAccelerator::SupportedProfile in gpu_info.h.

gpu_info.h depended on media/video and media/ depended on gpu/.
gpu/ and media/ had a dependency cycle. Duplicate VEA::SupportedProfile
to gpu_info.h so gpu does not depend on media.

BUG=420801
TEST=Run apprtc loopback and ensure it uses HW encoder.

Committed: https://crrev.com/6dd8cbfeb1b6df605d24988264c775af25ac595b
Cr-Commit-Position: refs/heads/master@{#301048}

[wuchengli, 2014-10-20 09:42:46]

Patchset #1 (id:1) has been deleted

[wuchengli, 2014-10-20 13:44:04]

Patchset #1 (id:20001) has been deleted

[wuchengli, 2014-10-20 13:44:10]

Patchset #1 (id:40001) has been deleted

[wuchengli, 2014-10-20 13:46:13]

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

[wuchengli, 2014-10-20 13:46:13]

PTAL.

[piman, 2014-10-21 19:41:53]

https://codereview.chromium.org/668633002/diff/60001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/668633002/diff/60001/gpu/config/gpu_info.h#ne...
gpu/config/gpu_info.h:40: // This is media::VideoCodecProfile. Use int to avoid
the dependency on media.
Can you move the enum here?

[wuchengli, 2014-10-22 00:51:23]

wuchengli@chromium.org changed reviewers:
+ kenrb@chromium.org, scherkus@chromium.org

[wuchengli, 2014-10-22 00:51:24]

scherkus@ Onwer review for media/.
kenrb@ Owner review for gpu_messages.h.

Thanks.

[wuchengli, 2014-10-22 00:52:35]

On 2014/10/21 19:41:53, piman (Very slow to review) wrote:
> https://codereview.chromium.org/668633002/diff/60001/gpu/config/gpu_info.h
> File gpu/config/gpu_info.h (right):
> 
>
https://codereview.chromium.org/668633002/diff/60001/gpu/config/gpu_info.h#ne...
> gpu/config/gpu_info.h:40: // This is media::VideoCodecProfile. Use int to
avoid
> the dependency on media.
> Can you move the enum here?
Software decoders (like ffmpeg and media/formats/webm/) also uses
VideoCodecProfile.
I feel VideoCodecProfile belongs to media/ more than gpu/. How do you think?
Pawel and Andrew. What's your take on this?

[piman, 2014-10-22 00:57:00]

On Tue, Oct 21, 2014 at 5:52 PM, <wuchengli@chromium.org> wrote:

> On 2014/10/21 19:41:53, piman (Very slow to review) wrote:
>
>> https://codereview.chromium.org/668633002/diff/60001/gpu/
>> config/gpu_info.h
>> File gpu/config/gpu_info.h (right):
>>
>
>
> https://codereview.chromium.org/668633002/diff/60001/gpu/
> config/gpu_info.h#newcode40
>
>> gpu/config/gpu_info.h:40: // This is media::VideoCodecProfile. Use int to
>>
> avoid
>
>> the dependency on media.
>> Can you move the enum here?
>>
> Software decoders (like ffmpeg and media/formats/webm/) also uses
> VideoCodecProfile.
> I feel VideoCodecProfile belongs to media/ more than gpu/. How do you
> think?
>

Then add a different enum in gpu with 1:1 mapping?

Enums should be enums, not ints. Otherwise it's impossible to verify at IPC
level that they have legitimate values, and can cause security issues later.


> Pawel and Andrew. What's your take on this?
>
>
> https://codereview.chromium.org/668633002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wuchengli, 2014-10-22 01:16:34]

On 2014/10/22 00:57:00, piman (Very slow to review) wrote:
> On Tue, Oct 21, 2014 at 5:52 PM, <mailto:wuchengli@chromium.org> wrote:
> 
> > On 2014/10/21 19:41:53, piman (Very slow to review) wrote:
> >
> >> https://codereview.chromium.org/668633002/diff/60001/gpu/
> >> config/gpu_info.h
> >> File gpu/config/gpu_info.h (right):
> >>
> >
> >
> > https://codereview.chromium.org/668633002/diff/60001/gpu/
> > config/gpu_info.h#newcode40
> >
> >> gpu/config/gpu_info.h:40: // This is media::VideoCodecProfile. Use int to
> >>
> > avoid
> >
> >> the dependency on media.
> >> Can you move the enum here?
> >>
> > Software decoders (like ffmpeg and media/formats/webm/) also uses
> > VideoCodecProfile.
> > I feel VideoCodecProfile belongs to media/ more than gpu/. How do you
> > think?
> >
> 
> Then add a different enum in gpu with 1:1 mapping?
> 
> Enums should be enums, not ints. Otherwise it's impossible to verify at IPC
> level that they have legitimate values, and can cause security issues later.
This is a good point. Let me think about it.
> 
> 
> > Pawel and Andrew. What's your take on this?
> >
> >
> > https://codereview.chromium.org/668633002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[wuchengli, 2014-10-22 08:48:31]

I found duplicating SupportedProfiles in gpu_info.h was more concise. PTAL.

[piman, 2014-10-22 20:46:31]

LGTM if adding static_asserts

https://codereview.chromium.org/668633002/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right):

https://codereview.chromium.org/668633002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_video_encode_accelerator_host.cc:88:
static_cast<media::VideoCodecProfile>(gpu_profiles[i].profile);
Can you add a list of static_asserts that check that the enums match? Including
min/max so that if someone adds a new value in one they'll know to change the
other one.

static_assert(media::VIDEO_CODEC_PROFILE_MAX == gpu::VIDEO_CODEC_PROFILE_MAX,
"VIDEO_CODEC_PROFILE_MAX value must match in media and gpu")' and so on.

[scherkus (not reviewing), 2014-10-22 22:46:23]

lgtm

[wuchengli, 2014-10-23 02:16:45]

kenrb@ Owner review for gpu_messages.h. Thanks.

[wuchengli, 2014-10-23 03:48:13]

On 2014/10/22 20:46:31, piman (Very slow to review) wrote:
> LGTM if adding static_asserts
> 
>
https://codereview.chromium.org/668633002/diff/100001/content/common/gpu/clie...
> File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right):
> 
>
https://codereview.chromium.org/668633002/diff/100001/content/common/gpu/clie...
> content/common/gpu/client/gpu_video_encode_accelerator_host.cc:88:
> static_cast<media::VideoCodecProfile>(gpu_profiles[i].profile);
> Can you add a list of static_asserts that check that the enums match?
Including
> min/max so that if someone adds a new value in one they'll know to change the
> other one.
> 
> static_assert(media::VIDEO_CODEC_PROFILE_MAX == gpu::VIDEO_CODEC_PROFILE_MAX,
> "VIDEO_CODEC_PROFILE_MAX value must match in media and gpu")' and so on.
Good suggestion. Done.

[piman, 2014-10-23 03:58:02]

lgtm

[kenrb, 2014-10-23 16:09:36]

ipc lgtm

[wuchengli, 2014-10-24 03:26:18]

The CQ bit was checked by wuchengli@chromium.org

[commit-bot: I haz the power, 2014-10-24 03:27:35]

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

[commit-bot: I haz the power, 2014-10-24 04:54:24]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2014-10-24 04:55:10]

Patchset 5 (id:??) landed as
https://crrev.com/6dd8cbfeb1b6df605d24988264c775af25ac595b
Cr-Commit-Position: refs/heads/master@{#301048}