Add VEA supported profiles to GPUInfo.

Add VEA supported profiles to GPUInfo.

Originally we use a static GetSupportedProfiles for each VEA
implementation to get a list of supported codec profiles. But
different devices of the same platform can support different
profiles. To return a correct list, they are dynamically
detected in runtime and added to GPUInfo.

BUG=350197
TEST=Run apprtc loopback and Hangout on peach pit.
Run gpu_info_unittest.

Committed: https://crrev.com/79808322a289e4eb20627be3637a08245c15ea3a
Cr-Commit-Position: refs/heads/master@{#296147}

[wuchengli, 2014-09-16 09:02:08]

Patchset #1 (id:1) has been deleted

[wuchengli, 2014-09-16 09:02:57]

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

[wuchengli, 2014-09-16 09:03:31]

This is a draft. PTAL.

[wuchengli, 2014-09-18 03:07:59]

Patchset #2 (id:40001) has been deleted

[wuchengli, 2014-09-18 03:09:18]

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

[wuchengli, 2014-09-18 03:12:49]

Patchset #2 (id:60001) has been deleted

[wuchengli, 2014-09-18 03:16:16]

wuchengli@chromium.org changed reviewers:
+ jln@chromium.org

[wuchengli, 2014-09-18 03:16:16]

PTAL.
piman@ Owner review for content/
scherkus@ Owner review for media/ and content/renderer/media/
jln@ Owner review for gpu_messages.h
Thanks.

[kcwu, 2014-09-18 05:18:35]

lgtm

[piman, 2014-09-18 18:11:51]

A sync message, especially to the GPU main thread can be costly...

https://codereview.chromium.org/568413002/diff/80001/content/common/gpu/gpu_m...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/568413002/diff/80001/content/common/gpu/gpu_m...
content/common/gpu/gpu_messages.h:582:
std::vector<media::VideoEncodeAccelerator::SupportedProfile> /* profiles */)
Would it be possible to combine this with the
GpuCommandBufferMsg_CreateVideoEncoder message?
Today you need 2 sync messages to figure out the supported profiles, and they're
costly.

But if you return them with the Create IPC, and you get everything in one go.

https://codereview.chromium.org/568413002/diff/80001/content/public/renderer/...
File content/public/renderer/video_encode_accelerator.cc (right):

https://codereview.chromium.org/568413002/diff/80001/content/public/renderer/...
content/public/renderer/video_encode_accelerator.cc:60: return profiles;
I think you want to cache this. This function is called in several innocuous
places, which don't expect it to be costly.

https://codereview.chromium.org/568413002/diff/80001/content/renderer/media/r...
File content/renderer/media/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/568413002/diff/80001/content/renderer/media/r...
content/renderer/media/rtc_video_encoder_factory.cc:78: waiter.Wait();
I don't think the RTCVideoEncoderFactory is created lazily?
I don't think we would want an extra sync round trip (or 2!) to the GPU process
at initialization time - this would impact page load time.

Can we figure out codecs_ lazily?

[wuchengli, 2014-09-19 10:33:10]

Patchset #4 (id:120001) has been deleted

[kcwu, 2014-09-19 11:15:55]

https://codereview.chromium.org/568413002/diff/140001/content/public/renderer...
File content/public/renderer/video_encode_accelerator.cc (right):

https://codereview.chromium.org/568413002/diff/140001/content/public/renderer...
content/public/renderer/video_encode_accelerator.cc:51:
std::vector<media::VideoEncodeAccelerator::SupportedProfile>
Since you cached the result to global variable, change the return type to const
reference to avoid copy?

https://codereview.chromium.org/568413002/diff/140001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/568413002/diff/140001/content/renderer/media/...
content/renderer/media/rtc_video_encoder_factory.cc:52: it++) {
++it

[wuchengli, 2014-09-19 14:27:25]

piman@ PTAL.

https://codereview.chromium.org/568413002/diff/80001/content/common/gpu/gpu_m...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/568413002/diff/80001/content/common/gpu/gpu_m...
content/common/gpu/gpu_messages.h:582:
std::vector<media::VideoEncodeAccelerator::SupportedProfile> /* profiles */)
On 2014/09/18 18:11:51, piman (Very slow to review) wrote:
> Would it be possible to combine this with the
> GpuCommandBufferMsg_CreateVideoEncoder message?
> Today you need 2 sync messages to figure out the supported profiles, and
they're
> costly.
We only need one sync message to get the supported profiles.
GpuCommandBufferMsg_CreateVideoEncoder corresponds to
VideoEncodeAccelerator::Initialize, which is not needed to get the profiles.
> 
> But if you return them with the Create IPC, and you get everything in one go.
WebRTC and Cast need to know the supported profiles before creating video
encoders. If we combine these two IPC, the client still needs 2 sync messages -
one for getting profiles and one for creating encoder.

https://codereview.chromium.org/568413002/diff/80001/content/public/renderer/...
File content/public/renderer/video_encode_accelerator.cc (right):

https://codereview.chromium.org/568413002/diff/80001/content/public/renderer/...
content/public/renderer/video_encode_accelerator.cc:60: return profiles;
On 2014/09/18 18:11:51, piman (Very slow to review) wrote:
> I think you want to cache this. This function is called in several innocuous
> places, which don't expect it to be costly.

Done.

https://codereview.chromium.org/568413002/diff/80001/content/renderer/media/r...
File content/renderer/media/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/568413002/diff/80001/content/renderer/media/r...
content/renderer/media/rtc_video_encoder_factory.cc:78: waiter.Wait();
On 2014/09/18 18:11:51, piman (Very slow to review) wrote:
> I don't think the RTCVideoEncoderFactory is created lazily?
RTCVideoEncoderFactory is created in PeerConnectionDependencyFactory and before
encoders are created. I don't know when PCDF is created. I guess it would impact
page load time. 
> I don't think we would want an extra sync round trip (or 2!) to the GPU
process
> at initialization time - this would impact page load time.
> 
> Can we figure out codecs_ lazily?
WebRTC already has a callback to indicate the codecs are ready --
OnCodecsAvailable. I've change the code to use it. OnCodecsAvailable is not used
before. Hopefully WebRTC has no bugs about it.

https://codereview.chromium.org/568413002/diff/140001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/568413002/diff/140001/content/renderer/media/...
content/renderer/media/rtc_video_encoder_factory.cc:81: base::Unretained(this),
The constructor and destructor run on the child thread. Here this posts a task
to media thread. The destructor makes sure the task has been run.

https://codereview.chromium.org/568413002/diff/140001/content/renderer/media/...
content/renderer/media/rtc_video_encoder_factory.cc:86:
weak_factory_.InvalidateWeakPtrs();
Invalidate the weak pointer in case the codecs_ has not constructed yet.

[piman, 2014-09-19 18:06:56]

On Fri, Sep 19, 2014 at 7:27 AM, <wuchengli@chromium.org> wrote:

> piman@ PTAL.
>
>
> https://codereview.chromium.org/568413002/diff/80001/
> content/common/gpu/gpu_messages.h
> File content/common/gpu/gpu_messages.h (right):
>
> https://codereview.chromium.org/568413002/diff/80001/
> content/common/gpu/gpu_messages.h#newcode582
> content/common/gpu/gpu_messages.h:582:
> std::vector<media::VideoEncodeAccelerator::SupportedProfile> /* profiles
> */)
> On 2014/09/18 18:11:51, piman (Very slow to review) wrote:
>
>> Would it be possible to combine this with the
>> GpuCommandBufferMsg_CreateVideoEncoder message?
>> Today you need 2 sync messages to figure out the supported profiles,
>>
> and they're
>
>> costly.
>>
> We only need one sync message to get the supported profiles.
> GpuCommandBufferMsg_CreateVideoEncoder corresponds to
> VideoEncodeAccelerator::Initialize, which is not needed to get the
> profiles.
>
>  But if you return them with the Create IPC, and you get everything in
>>
> one go.
> WebRTC and Cast need to know the supported profiles before creating
> video encoders. If we combine these two IPC, the client still needs 2
> sync messages - one for getting profiles and one for creating encoder.


Oh, I see, this is very confusing.
GpuCommandBufferMsg_VideoEncodeAcceleratorGetSupportedProfiles takes the
routing id not of the video encoder, but of the command buffer. The code
suggests you're creating a video encoder and calling a method on it. This
is really a call to the command buffer not the encoder. Can you move the
call to CommandBufferProxyImpl, so that you don't need to create an encoder?


>
> https://codereview.chromium.org/568413002/diff/80001/
> content/public/renderer/video_encode_accelerator.cc
> File content/public/renderer/video_encode_accelerator.cc (right):
>
> https://codereview.chromium.org/568413002/diff/80001/
> content/public/renderer/video_encode_accelerator.cc#newcode60
> content/public/renderer/video_encode_accelerator.cc:60: return profiles;
> On 2014/09/18 18:11:51, piman (Very slow to review) wrote:
>
>> I think you want to cache this. This function is called in several
>>
> innocuous
>
>> places, which don't expect it to be costly.
>>
>
> Done.
>
> https://codereview.chromium.org/568413002/diff/80001/
> content/renderer/media/rtc_video_encoder_factory.cc
> File content/renderer/media/rtc_video_encoder_factory.cc (right):
>
> https://codereview.chromium.org/568413002/diff/80001/
> content/renderer/media/rtc_video_encoder_factory.cc#newcode78
> content/renderer/media/rtc_video_encoder_factory.cc:78: waiter.Wait();
> On 2014/09/18 18:11:51, piman (Very slow to review) wrote:
>
>> I don't think the RTCVideoEncoderFactory is created lazily?
>>
> RTCVideoEncoderFactory is created in PeerConnectionDependencyFactory and
> before encoders are created. I don't know when PCDF is created. I guess
> it would impact page load time.


AFAICT, it's created with the RenderThreadImpl, so it would impact PLT, on
all pages, even the ones that don't use video encode/decode. That's bad.

How about the suggestion to pass the list of codecs with the GPU info? The
channel creation is already on the critical path for page load, so this
wouldn't add any round trip.


>  I don't think we would want an extra sync round trip (or 2!) to the
>>
> GPU process
>
>> at initialization time - this would impact page load time.
>>
>
>  Can we figure out codecs_ lazily?
>>
> WebRTC already has a callback to indicate the codecs are ready --
> OnCodecsAvailable. I've change the code to use it. OnCodecsAvailable is
> not used before. Hopefully WebRTC has no bugs about it.
>
> https://codereview.chromium.org/568413002/diff/140001/
> content/renderer/media/rtc_video_encoder_factory.cc
> File content/renderer/media/rtc_video_encoder_factory.cc (right):
>
> https://codereview.chromium.org/568413002/diff/140001/
> content/renderer/media/rtc_video_encoder_factory.cc#newcode81
> content/renderer/media/rtc_video_encoder_factory.cc:81:
> base::Unretained(this),
> The constructor and destructor run on the child thread. Here this posts
> a task to media thread. The destructor makes sure the task has been run.
>
> https://codereview.chromium.org/568413002/diff/140001/
> content/renderer/media/rtc_video_encoder_factory.cc#newcode86
> content/renderer/media/rtc_video_encoder_factory.cc:86:
> weak_factory_.InvalidateWeakPtrs();
> Invalidate the weak pointer in case the codecs_ has not constructed yet.
>
> https://codereview.chromium.org/568413002/
>

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

[wuchengli, 2014-09-19 22:35:57]

> > On 2014/09/18 18:11:51, piman (Very slow to review) wrote:
> >
> >> Would it be possible to combine this with the
> >> GpuCommandBufferMsg_CreateVideoEncoder message?
> >> Today you need 2 sync messages to figure out the supported profiles,
> >>
> > and they're
> >
> >> costly.
> >>
> > We only need one sync message to get the supported profiles.
> > GpuCommandBufferMsg_CreateVideoEncoder corresponds to
> > VideoEncodeAccelerator::Initialize, which is not needed to get the
> > profiles.
> >
> >  But if you return them with the Create IPC, and you get everything in
> >>
> > one go.
> > WebRTC and Cast need to know the supported profiles before creating
> > video encoders. If we combine these two IPC, the client still needs 2
> > sync messages - one for getting profiles and one for creating encoder.
> 
> 
> Oh, I see, this is very confusing.
> GpuCommandBufferMsg_VideoEncodeAcceleratorGetSupportedProfiles takes the
> routing id not of the video encoder, but of the command buffer. The code
> suggests you're creating a video encoder and calling a method on it. This
> is really a call to the command buffer not the encoder. Can you move the
> call to CommandBufferProxyImpl, so that you don't need to create an encoder?
I see how to do it in the next patchset. 
> 
> 
> > https://codereview.chromium.org/568413002/diff/80001/
> > content/renderer/media/rtc_video_encoder_factory.cc
> > File content/renderer/media/rtc_video_encoder_factory.cc (right):
> >
> > https://codereview.chromium.org/568413002/diff/80001/
> > content/renderer/media/rtc_video_encoder_factory.cc#newcode78
> > content/renderer/media/rtc_video_encoder_factory.cc:78: waiter.Wait();
> > On 2014/09/18 18:11:51, piman (Very slow to review) wrote:
> >
> >> I don't think the RTCVideoEncoderFactory is created lazily?
> >>
> > RTCVideoEncoderFactory is created in PeerConnectionDependencyFactory and
> > before encoders are created. I don't know when PCDF is created. I guess
> > it would impact page load time.
> 
> 
> AFAICT, it's created with the RenderThreadImpl, so it would impact PLT, on
> all pages, even the ones that don't use video encode/decode. That's bad.
I looked at the code. RTCVideoEncoderFactory is created in
PeerConnectionDepdencyFactory::CreatePeerConnectionFactory(). It is called
when a video capturer, a video source, a media stream, an audio source, or
a peer connection is created. So it wouldn't impact PLT of all pages. I
tested it. RTCVideoEncoderFactory was not created when visiting google.com.
It was created when visiting apprtc.appspot.com.

Also, since RTCVideoEncoderFactory constructor does not wait until getting
the profiles. PLT of all pages won't be impacted.
> 
> How about the suggestion to pass the list of codecs with the GPU info? The
> channel creation is already on the critical path for page load, so this
> wouldn't add any round trip.
I'm not very familiar with GPU info. One thing I'm not sure is whether VEA
profiles belong to GPU info. VEA is run in GPU process but it actually uses
other hardware to do the encoding on ARM.

[piman, 2014-09-19 22:46:20]

On Fri, Sep 19, 2014 at 3:35 PM, <wuchengli@chromium.org> wrote:

> > On 2014/09/18 18:11:51, piman (Very slow to review) wrote:
>> >
>> >> Would it be possible to combine this with the
>> >> GpuCommandBufferMsg_CreateVideoEncoder message?
>> >> Today you need 2 sync messages to figure out the supported profiles,
>> >>
>> > and they're
>> >
>> >> costly.
>> >>
>> > We only need one sync message to get the supported profiles.
>> > GpuCommandBufferMsg_CreateVideoEncoder corresponds to
>> > VideoEncodeAccelerator::Initialize, which is not needed to get the
>> > profiles.
>> >
>> >  But if you return them with the Create IPC, and you get everything in
>> >>
>> > one go.
>> > WebRTC and Cast need to know the supported profiles before creating
>> > video encoders. If we combine these two IPC, the client still needs 2
>> > sync messages - one for getting profiles and one for creating encoder.
>>
>
>
>  Oh, I see, this is very confusing.
>> GpuCommandBufferMsg_VideoEncodeAcceleratorGetSupportedProfiles takes the
>> routing id not of the video encoder, but of the command buffer. The code
>> suggests you're creating a video encoder and calling a method on it. This
>> is really a call to the command buffer not the encoder. Can you move the
>> call to CommandBufferProxyImpl, so that you don't need to create an
>> encoder?
>>
> I see how to do it in the next patchset.
>
>
>  > https://codereview.chromium.org/568413002/diff/80001/
>> > content/renderer/media/rtc_video_encoder_factory.cc
>> > File content/renderer/media/rtc_video_encoder_factory.cc (right):
>> >
>> > https://codereview.chromium.org/568413002/diff/80001/
>> > content/renderer/media/rtc_video_encoder_factory.cc#newcode78
>> > content/renderer/media/rtc_video_encoder_factory.cc:78: waiter.Wait();
>> > On 2014/09/18 18:11:51, piman (Very slow to review) wrote:
>> >
>> >> I don't think the RTCVideoEncoderFactory is created lazily?
>> >>
>> > RTCVideoEncoderFactory is created in PeerConnectionDependencyFactory
>> and
>> > before encoders are created. I don't know when PCDF is created. I guess
>> > it would impact page load time.
>>
>
>
>  AFAICT, it's created with the RenderThreadImpl, so it would impact PLT, on
>> all pages, even the ones that don't use video encode/decode. That's bad.
>>
> I looked at the code. RTCVideoEncoderFactory is created in
> PeerConnectionDepdencyFactory::CreatePeerConnectionFactory(). It is called
> when a video capturer, a video source, a media stream, an audio source, or
> a peer connection is created. So it wouldn't impact PLT of all pages. I
> tested it. RTCVideoEncoderFactory was not created when visiting google.com
> .
> It was created when visiting apprtc.appspot.com.
>
> Also, since RTCVideoEncoderFactory constructor does not wait until getting
> the profiles. PLT of all pages won't be impacted.
>
>  How about the suggestion to pass the list of codecs with the GPU info? The
>> channel creation is already on the critical path for page load, so this
>> wouldn't add any round trip.
>>
> I'm not very familiar with GPU info. One thing I'm not sure is whether VEA
> profiles belong to GPU info. VEA is run in GPU process but it actually uses
> other hardware to do the encoding on ARM.
>

GPUInfo is the structure that's created by the GPU process on first
connection that contains enough information such as GPU model or extensions
to decide things like blacklists. It flows all the way to
the GpuChannelHosts when they're created.
I think it's exactly where this information should be. Then you don't need
all sorts of complexity wrt sync messages and round trips and caching and
lazy init.


>
> https://codereview.chromium.org/568413002/
>

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

[wuchengli, 2014-09-22 14:21:29]

Patchset #7 (id:200001) has been deleted

[wuchengli, 2014-09-22 14:28:46]

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

[wuchengli, 2014-09-22 14:34:02]

Patchset #7 (id:220001) has been deleted

[wuchengli, 2014-09-22 14:37:44]

PTAL. After using GPUInfo, the code is simpler! The only concern is this adds
the dependency of media/video to gpu/config. Is it OK?

kenrb@ OWNER review for gpu_messages.h. The nickname of jln says he'll be slow
on IPC reviews.

scherkus@ OWNER review for media/. Let me know if you have time to review this.

Thanks.

[kenrb, 2014-09-22 15:25:11]

ipc lgtm

https://codereview.chromium.org/568413002/diff/240001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/568413002/diff/240001/content/renderer/media/...
content/renderer/media/rtc_video_encoder_factory.cc:62: }  // anonymouse
namespace
?

[scherkus (not reviewing), 2014-09-22 17:50:44]

lgtm

[piman, 2014-09-22 19:32:02]

gpu/config depending on media/video isn't a fundamental issue, but it may be
causing trouble currently with the component build, because gpu_config is folded
into the gpu component, and media depends on gpu.

That said, if all you need is the enum, you shouldn't need to add a build
dependency.

LGTM if build things figured out (gn bots are failing).

[wuchengli, 2014-09-22 22:02:44]

On 2014/09/22 19:32:02, piman (Very slow to review) wrote:
> gpu/config depending on media/video isn't a fundamental issue, but it may be
> causing trouble currently with the component build, because gpu_config is
folded
> into the gpu component, and media depends on gpu.
Hmm. Do you mean we cannot have mutual dependency between two components
with the component build? What's the proper way to fix it? Making media/video
 a component? I've been using component build and I don't want to break it.

Another question is gpu_info.h includes media/video/video_encode_accelerator.h
and
video_encode_accelerator.h includes some files from media/base. Do I need to add
media/video only or both media/video and media/base in gpu/config/DEPS?
> 
> That said, if all you need is the enum, you shouldn't need to add a build
> dependency.
We need every field in SupportedProfile.
> 
> LGTM if build things figured out (gn bots are failing).
I'll look at gn bots later today.

Btw, I don't know why BeginGPUDevice and EndGPUDevice in
devtools_system_info_handler.cc are empty. They are not used elsewhere.
I assume {Begin,End}VideoEncodeAcceleratorSupportedProfile should be also
empty and I don't need to test it.

[wuchengli, 2014-09-22 22:14:14]

On 2014/09/22 22:02:44, wuchengli wrote:
> On 2014/09/22 19:32:02, piman (Very slow to review) wrote:
> > gpu/config depending on media/video isn't a fundamental issue, but it may be
> > causing trouble currently with the component build, because gpu_config is
> folded
> > into the gpu component, and media depends on gpu.
> Hmm. Do you mean we cannot have mutual dependency between two components
> with the component build? What's the proper way to fix it? Making media/video
>  a component? I've been using component build and I don't want to break it.
> 
> Another question is gpu_info.h includes media/video/video_encode_accelerator.h
> and
> video_encode_accelerator.h includes some files from media/base. Do I need to
add
> media/video only or both media/video and media/base in gpu/config/DEPS?
> > 
> > That said, if all you need is the enum, you shouldn't need to add a build
> > dependency.
> We need every field in SupportedProfile.
> > 
> > LGTM if build things figured out (gn bots are failing).
> I'll look at gn bots later today.
> 
> Btw, I don't know why BeginGPUDevice and EndGPUDevice in
> devtools_system_info_handler.cc are empty. They are not used elsewhere.
> I assume {Begin,End}VideoEncodeAcceleratorSupportedProfile should be also
> empty and I don't need to test it.
How do I trigger the path of AuxGPUInfoEnumerator in
devtools_system_info_handler.cc? Inputting a command in Tools->Developer tools?

[piman, 2014-09-22 22:15:18]

On Mon, Sep 22, 2014 at 3:02 PM, <wuchengli@chromium.org> wrote:

> On 2014/09/22 19:32:02, piman (Very slow to review) wrote:
>
>> gpu/config depending on media/video isn't a fundamental issue, but it may
>> be
>> causing trouble currently with the component build, because gpu_config is
>>
> folded
>
>> into the gpu component, and media depends on gpu.
>>
> Hmm. Do you mean we cannot have mutual dependency between two components
> with the component build?


We cannot have circular dependencies.


> What's the proper way to fix it? Making media/video
>  a component? I've been using component build and I don't want to break it.
>

I was thinking making gpu_config a separate component from gpu.


> Another question is gpu_info.h includes media/video/video_encode_
> accelerator.h
> and
> video_encode_accelerator.h includes some files from media/base. Do I need
> to add
> media/video only or both media/video and media/base in gpu/config/DEPS?


For DEPS, you only need direct include paths, i.e. media/video is enough.


> That said, if all you need is the enum, you shouldn't need to add a build
>> dependency.
>>
> We need every field in SupportedProfile.


It's an edge case. I think technically, you don't need a build-time
dependency currently, because SupportedProfile is defined entirely in the
header.
But the day someone adds a non-trivial field that needs a constructor in
the .cc file, it would break.

Safest is probably to add the dependency.


>
>  LGTM if build things figured out (gn bots are failing).
>>
> I'll look at gn bots later today.
>
> Btw, I don't know why BeginGPUDevice and EndGPUDevice in
> devtools_system_info_handler.cc are empty. They are not used elsewhere.
> I assume {Begin,End}VideoEncodeAcceleratorSupportedProfile should be also
> empty and I don't need to test it.
>
>
> https://codereview.chromium.org/568413002/
>

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

[piman, 2014-09-22 22:15:59]

piman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[piman, 2014-09-22 22:16:00]

+pfeldman for devtools stuff (see questions upthread)

[Pawel Osciak, 2014-09-23 00:08:49]

Just some nits.
Could you also please add this call to veatest and ASSERT that the codec that
the test expects is supported?

https://codereview.chromium.org/568413002/diff/260001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/568413002/diff/260001/content/renderer/media/...
content/renderer/media/rtc_video_encoder_factory.cc:62: }  // anonymouse
namespace
s/anonymouse/anonymous/

https://codereview.chromium.org/568413002/diff/260001/gpu/config/DEPS
File gpu/config/DEPS (right):

https://codereview.chromium.org/568413002/diff/260001/gpu/config/DEPS#newcode4
gpu/config/DEPS:4: "+media/video",  # For
VideoEncodeAccelerator::SupportedProfile.
GetSupportedProfiles

https://codereview.chromium.org/568413002/diff/260001/media/cast/test/fake_vi...
File media/cast/test/fake_video_encode_accelerator.cc (right):

https://codereview.chromium.org/568413002/diff/260001/media/cast/test/fake_vi...
media/cast/test/fake_video_encode_accelerator.cc:37: return
std::vector<VideoEncodeAccelerator::SupportedProfile>();
Would be nice to add this query to cast unittests, doesn't have to be a part of
this cl.

[wuchengli, 2014-09-23 03:37:05]

On 2014/09/23 00:08:49, Pawel Osciak wrote:
> Just some nits.
> Could you also please add this call to veatest and ASSERT that the codec that
> the test expects is supported?
I found V4L2VideoEncodeAccelerator::GetSupportedProfiles would only return one
codec according to switches::kEnableWebRtcHWVp8Encoding. I'll follow up another
CL to remove kEnableWebRtcHWVp8Encoding and then add this call to veatest.

> 
>
https://codereview.chromium.org/568413002/diff/260001/content/renderer/media/...
> File content/renderer/media/rtc_video_encoder_factory.cc (right):
> 
>
https://codereview.chromium.org/568413002/diff/260001/content/renderer/media/...
> content/renderer/media/rtc_video_encoder_factory.cc:62: }  // anonymouse
> namespace
> s/anonymouse/anonymous/
> 
> https://codereview.chromium.org/568413002/diff/260001/gpu/config/DEPS
> File gpu/config/DEPS (right):
> 
> https://codereview.chromium.org/568413002/diff/260001/gpu/config/DEPS#newcode4
> gpu/config/DEPS:4: "+media/video",  # For
> VideoEncodeAccelerator::SupportedProfile.
> GetSupportedProfiles
> 
>
https://codereview.chromium.org/568413002/diff/260001/media/cast/test/fake_vi...
> File media/cast/test/fake_video_encode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/568413002/diff/260001/media/cast/test/fake_vi...
> media/cast/test/fake_video_encode_accelerator.cc:37: return
> std::vector<VideoEncodeAccelerator::SupportedProfile>();
> Would be nice to add this query to cast unittests, doesn't have to be a part
of
> this cl.

[wuchengli, 2014-09-23 03:37:25]

I'll file a bug for making gpu/config a component.

https://codereview.chromium.org/568413002/diff/240001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/568413002/diff/240001/content/renderer/media/...
content/renderer/media/rtc_video_encoder_factory.cc:62: }  // anonymouse
namespace
On 2014/09/22 15:25:11, kenrb wrote:
> ?
Fixed.

https://codereview.chromium.org/568413002/diff/260001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder_factory.cc (right):

https://codereview.chromium.org/568413002/diff/260001/content/renderer/media/...
content/renderer/media/rtc_video_encoder_factory.cc:62: }  // anonymouse
namespace
On 2014/09/23 00:08:48, Pawel Osciak wrote:
> s/anonymouse/anonymous/

Done.

https://codereview.chromium.org/568413002/diff/260001/gpu/config/DEPS
File gpu/config/DEPS (right):

https://codereview.chromium.org/568413002/diff/260001/gpu/config/DEPS#newcode4
gpu/config/DEPS:4: "+media/video",  # For
VideoEncodeAccelerator::SupportedProfile.
On 2014/09/23 00:08:49, Pawel Osciak wrote:
> GetSupportedProfiles
gpu/config uses VideoEncodeAccelerator::SupportedProfile. It doesn't call
VideoEncodeAccelerator::GetSupportedProfiles().

https://codereview.chromium.org/568413002/diff/260001/media/cast/test/fake_vi...
File media/cast/test/fake_video_encode_accelerator.cc (right):

https://codereview.chromium.org/568413002/diff/260001/media/cast/test/fake_vi...
media/cast/test/fake_video_encode_accelerator.cc:37: return
std::vector<VideoEncodeAccelerator::SupportedProfile>();
On 2014/09/23 00:08:49, Pawel Osciak wrote:
> Would be nice to add this query to cast unittests, doesn't have to be a part
of
> this cl.

Acknowledged.

[Pawel Osciak, 2014-09-23 04:10:29]

On 2014/09/23 03:37:05, wuchengli wrote:
> On 2014/09/23 00:08:49, Pawel Osciak wrote:
> > Just some nits.
> > Could you also please add this call to veatest and ASSERT that the codec
that
> > the test expects is supported?
> I found V4L2VideoEncodeAccelerator::GetSupportedProfiles would only return one
> codec according to switches::kEnableWebRtcHWVp8Encoding. I'll follow up
another
> CL to remove kEnableWebRtcHWVp8Encoding and then add this call to veatest.

Please do not remove kEnableWebRtcHWVp8Encoding. If you remove it, platforms
that don't support VP8 will report it.

[Pawel Osciak, 2014-09-23 04:13:19]

lgtm

https://codereview.chromium.org/568413002/diff/260001/gpu/config/DEPS
File gpu/config/DEPS (right):

https://codereview.chromium.org/568413002/diff/260001/gpu/config/DEPS#newcode4
gpu/config/DEPS:4: "+media/video",  # For
VideoEncodeAccelerator::SupportedProfile.
On 2014/09/23 03:37:25, wuchengli wrote:
> On 2014/09/23 00:08:49, Pawel Osciak wrote:
> > GetSupportedProfiles
> gpu/config uses VideoEncodeAccelerator::SupportedProfile. It doesn't call
> VideoEncodeAccelerator::GetSupportedProfiles().

Acknowledged.

[wuchengli, 2014-09-23 04:53:32]

The CQ bit was checked by wuchengli@chromium.org

[commit-bot: I haz the power, 2014-09-23 04:54:13]

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

[commit-bot: I haz the power, 2014-09-23 05:58:22]

Committed patchset #9 (id:280001) as d05bac4c954aa0c07ea55f8931efdbed062165bc

[commit-bot: I haz the power, 2014-09-23 05:58:54]

Patchset 9 (id:??) landed as
https://crrev.com/79808322a289e4eb20627be3637a08245c15ea3a
Cr-Commit-Position: refs/heads/master@{#296147}

[wuchengli, 2014-09-25 13:52:54]

Patchset #10 (id:300001) has been deleted

[jamesr, 2014-10-06 18:11:16]

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

[jamesr, 2014-10-06 18:11:17]

https://codereview.chromium.org/568413002/diff/280001/gpu/config/DEPS
File gpu/config/DEPS (right):

https://codereview.chromium.org/568413002/diff/280001/gpu/config/DEPS#newcode4
gpu/config/DEPS:4: "+media/video",  # For
VideoEncodeAccelerator::SupportedProfile.
this is really crappy - it creates a cycle between the gpu/ and media/
components and makes it harder to understand the dependency graph.  Could you do
this in a better way?

[wuchengli, 2014-10-06 22:03:48]

https://codereview.chromium.org/568413002/diff/280001/gpu/config/DEPS
File gpu/config/DEPS (right):

https://codereview.chromium.org/568413002/diff/280001/gpu/config/DEPS#newcode4
gpu/config/DEPS:4: "+media/video",  # For
VideoEncodeAccelerator::SupportedProfile.
On 2014/10/06 18:11:17, jamesr wrote:
> this is really crappy - it creates a cycle between the gpu/ and media/
> components and makes it harder to understand the dependency graph.  Could you
do
> this in a better way?
I just filed http://crbug.com/420801. The suggestion from piman was to make
gpu/config a component. Let me know if that's the right way on the bug. Thanks.