Limit the number of ARC codec

Limit the number of ARC codec

BUG=b/28157714
TEST=create too much ARC codecs and it should fail when reach limit

Committed: https://crrev.com/1b88e4a9efae89275f1ae2425768d39988d28e45
Cr-Commit-Position: refs/heads/master@{#398218}

[kcwu, 2016-06-02 14:55:33]

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

[owenlin_google, 2016-06-03 02:26:54]

owenlin@google.com changed reviewers:
+ owenlin@google.com

[owenlin_google, 2016-06-03 02:26:55]

lgtm

https://codereview.chromium.org/2036723002/diff/1/components/arc/common/video...
File components/arc/common/video_accelerator.mojom (right):

https://codereview.chromium.org/2036723002/diff/1/components/arc/common/video...
components/arc/common/video_accelerator.mojom:62:
Initialize@0(ArcVideoAcceleratorConfig config) => (bool result, [MinVersion=1]
Error error);
line too long?

[Pawel Osciak, 2016-06-03 03:56:18]

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/arc_gpu_video_de...
File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/arc_gpu_video_de...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:63: if (vda_) {
How do we guarantee this happens on the correct thread? Could we at least have a
DCHECK please?

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/arc_gpu_video_de...
File chrome/gpu/arc_gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/arc_gpu_video_de...
chrome/gpu/arc_gpu_video_decode_accelerator.h:123: // Global counter that keeps
track how many VDAs in use for ARC.
I think we mean ArcVideoDecodeAccelerator instead of arc codec everywhere?

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/arc_gpu_video_de...
chrome/gpu/arc_gpu_video_decode_accelerator.h:124: static int arc_codec_count_;
Please add a comment about locking here.

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/arc_video_accele...
File chrome/gpu/arc_video_accelerator.h (right):

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/arc_video_accele...
chrome/gpu/arc_video_accelerator.h:55: enum Error {
Should we call it Result instead of Error, if NO_ERROR is possible?

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/arc_video_accele...
chrome/gpu/arc_video_accelerator.h:109: // returns true iff initialization is
successful.
Please update documentation.

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/gpu_arc_video_se...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/gpu_arc_video_se...
chrome/gpu/gpu_arc_video_service.cc:145: callback.Run(true,
::arc::mojom::VideoAcceleratorService::Error::NO_ERROR);
Do we need true/false in the first argument, if we have NO_ERROR?

[kcwu, 2016-06-03 08:28:58]

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/gpu_arc_video_se...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/gpu_arc_video_se...
chrome/gpu/gpu_arc_video_service.cc:145: callback.Run(true,
::arc::mojom::VideoAcceleratorService::Error::NO_ERROR);
On 2016/06/03 03:56:18, Pawel Osciak wrote:
> Do we need true/false in the first argument, if we have NO_ERROR?

It's backward compatible if appending parameters, but change type is not. I'd
like to keep the boolean in order make it backward compatible. 

From previous experience, CrOS image may stick to certain version of chrome or
arc for a long time for some reason. I don't want to back arc video in
unfortunate period.

[kcwu, 2016-06-03 08:29:38]

On 2016/06/03 08:28:58, kcwu wrote:
>
https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/gpu_arc_video_se...
> File chrome/gpu/gpu_arc_video_service.cc (right):
> 
>
https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/gpu_arc_video_se...
> chrome/gpu/gpu_arc_video_service.cc:145: callback.Run(true,
> ::arc::mojom::VideoAcceleratorService::Error::NO_ERROR);
> On 2016/06/03 03:56:18, Pawel Osciak wrote:
> > Do we need true/false in the first argument, if we have NO_ERROR?
> 
> It's backward compatible if appending parameters, but change type is not. I'd
> like to keep the boolean in order make it backward compatible. 
> 
> From previous experience, CrOS image may stick to certain version of chrome or
> arc for a long time for some reason. I don't want to back arc video in
> unfortunate period.

s/back/break/

[kcwu, 2016-06-03 13:11:58]

Patchset #2 (id:20001) has been deleted

[kcwu, 2016-06-03 13:14:15]

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/arc_gpu_video_de...
File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/arc_gpu_video_de...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:63: if (vda_) {
On 2016/06/03 03:56:18, Pawel Osciak wrote:
> How do we guarantee this happens on the correct thread? Could we at least have
a
> DCHECK please?

Done.

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/arc_gpu_video_de...
File chrome/gpu/arc_gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/arc_gpu_video_de...
chrome/gpu/arc_gpu_video_decode_accelerator.h:123: // Global counter that keeps
track how many VDAs in use for ARC.
On 2016/06/03 03:56:18, Pawel Osciak wrote:
> I think we mean ArcVideoDecodeAccelerator instead of arc codec everywhere?

I revised all variable name and comments. 'client count' should be less
confusing.

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/arc_gpu_video_de...
chrome/gpu/arc_gpu_video_decode_accelerator.h:124: static int arc_codec_count_;
On 2016/06/03 03:56:18, Pawel Osciak wrote:
> Please add a comment about locking here.

Done.

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/arc_video_accele...
File chrome/gpu/arc_video_accelerator.h (right):

https://codereview.chromium.org/2036723002/diff/1/chrome/gpu/arc_video_accele...
chrome/gpu/arc_video_accelerator.h:55: enum Error {
On 2016/06/03 03:56:18, Pawel Osciak wrote:
> Should we call it Result instead of Error, if NO_ERROR is possible?

Done.

Note, OnError becomes:
void OnError(Result error);

https://codereview.chromium.org/2036723002/diff/1/components/arc/common/video...
File components/arc/common/video_accelerator.mojom (right):

https://codereview.chromium.org/2036723002/diff/1/components/arc/common/video...
components/arc/common/video_accelerator.mojom:62:
Initialize@0(ArcVideoAcceleratorConfig config) => (bool result, [MinVersion=1]
Error error);
On 2016/06/03 02:26:54, owenlin_google wrote:
> line too long?

Done.

[Pawel Osciak, 2016-06-06 05:49:13]

lgtm % nits

https://codereview.chromium.org/2036723002/diff/40001/chrome/gpu/arc_gpu_vide...
File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2036723002/diff/40001/chrome/gpu/arc_gpu_vide...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:23: // Maximum number of
concurrent ARC video client.
s/client/clients/

https://codereview.chromium.org/2036723002/diff/40001/chrome/gpu/arc_gpu_vide...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:26: const int
kMaxConcurrentClient = 8;
s/Client/Clients/

https://codereview.chromium.org/2036723002/diff/40001/chrome/gpu/arc_gpu_vide...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:121: DVLOG(5) << "Number of
concurrent ArcVideoAccelerator client: "
s/client/clients/

https://codereview.chromium.org/2036723002/diff/40001/chrome/gpu/arc_video_ac...
File chrome/gpu/arc_video_accelerator.h (right):

https://codereview.chromium.org/2036723002/diff/40001/chrome/gpu/arc_video_ac...
chrome/gpu/arc_video_accelerator.h:56: NO_ERROR = 0,
Hm, actually, now that this is a Result, we could just call this SUCCESS ?

https://codereview.chromium.org/2036723002/diff/40001/chrome/gpu/arc_video_ac...
chrome/gpu/arc_video_accelerator.h:84: // will not be reported here, but will be
indicated by a false return value
This also needs an update please.

[kcwu, 2016-06-06 06:53:12]

https://codereview.chromium.org/2036723002/diff/40001/chrome/gpu/arc_gpu_vide...
File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2036723002/diff/40001/chrome/gpu/arc_gpu_vide...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:23: // Maximum number of
concurrent ARC video client.
On 2016/06/06 05:49:12, Pawel Osciak wrote:
> s/client/clients/

Done.

https://codereview.chromium.org/2036723002/diff/40001/chrome/gpu/arc_gpu_vide...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:26: const int
kMaxConcurrentClient = 8;
On 2016/06/06 05:49:13, Pawel Osciak wrote:
> s/Client/Clients/

Done.

https://codereview.chromium.org/2036723002/diff/40001/chrome/gpu/arc_gpu_vide...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:121: DVLOG(5) << "Number of
concurrent ArcVideoAccelerator client: "
On 2016/06/06 05:49:12, Pawel Osciak wrote:
> s/client/clients/

Done.

https://codereview.chromium.org/2036723002/diff/40001/chrome/gpu/arc_video_ac...
File chrome/gpu/arc_video_accelerator.h (right):

https://codereview.chromium.org/2036723002/diff/40001/chrome/gpu/arc_video_ac...
chrome/gpu/arc_video_accelerator.h:56: NO_ERROR = 0,
On 2016/06/06 05:49:13, Pawel Osciak wrote:
> Hm, actually, now that this is a Result, we could just call this SUCCESS ?

Done.

https://codereview.chromium.org/2036723002/diff/40001/chrome/gpu/arc_video_ac...
chrome/gpu/arc_video_accelerator.h:84: // will not be reported here, but will be
indicated by a false return value
On 2016/06/06 05:49:13, Pawel Osciak wrote:
> This also needs an update please.

Done.

[Pawel Osciak, 2016-06-06 07:17:20]

Thanks, lgtm % tiny nit

https://codereview.chromium.org/2036723002/diff/60001/components/arc/common/v...
File components/arc/common/video_accelerator.mojom (right):

https://codereview.chromium.org/2036723002/diff/60001/components/arc/common/v...
components/arc/common/video_accelerator.mojom:65:
Initialize@7(ArcVideoAcceleratorConfig config) => (Result error);
s/error/result/ ?

[kcwu, 2016-06-06 07:30:06]

kcwu@chromium.org changed reviewers:
+ dcheng@chromium.org

[kcwu, 2016-06-06 07:30:07]

@dcheng, please OWNERS review for components/arc/common/video_accelerator.mojom

Thanks.

[kcwu, 2016-06-06 10:28:54]

Patchset #5 (id:100001) has been deleted

[dcheng, 2016-06-07 00:57:56]

https://codereview.chromium.org/2036723002/diff/80001/chrome/gpu/gpu_arc_vide...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/2036723002/diff/80001/chrome/gpu/gpu_arc_vide...
chrome/gpu/gpu_arc_video_service.cc:113:
static_cast<::arc::mojom::VideoAcceleratorService::Result>(error));
Until we have enum traits, can we static_assert somewhere that these enum values
actually match?

https://codereview.chromium.org/2036723002/diff/80001/chrome/gpu/gpu_arc_vide...
chrome/gpu/gpu_arc_video_service.cc:146:
static_cast<::arc::mojom::VideoAcceleratorService::Result>(result));
Ditto.

[kcwu, 2016-06-07 01:50:08]

https://codereview.chromium.org/2036723002/diff/80001/chrome/gpu/gpu_arc_vide...
File chrome/gpu/gpu_arc_video_service.cc (right):

https://codereview.chromium.org/2036723002/diff/80001/chrome/gpu/gpu_arc_vide...
chrome/gpu/gpu_arc_video_service.cc:113:
static_cast<::arc::mojom::VideoAcceleratorService::Result>(error));
On 2016/06/07 00:57:56, dcheng wrote:
> Until we have enum traits, can we static_assert somewhere that these enum
values
> actually match?

Done.

https://codereview.chromium.org/2036723002/diff/80001/chrome/gpu/gpu_arc_vide...
chrome/gpu/gpu_arc_video_service.cc:146:
static_cast<::arc::mojom::VideoAcceleratorService::Result>(result));
On 2016/06/07 00:57:56, dcheng wrote:
> Ditto.

Done.

[dcheng, 2016-06-07 02:25:31]

mojom lgtm

[kcwu, 2016-06-07 03:10:55]

The CQ bit was checked by kcwu@chromium.org

[kcwu, 2016-06-07 03:10:55]

The patchset sent to the CQ was uploaded after l-g-t-m from owenlin@google.com,
posciak@chromium.org
Link to the patchset: https://codereview.chromium.org/2036723002/#ps120001
(title: "address dcheng's comments: static_assert")

[commit-bot: I haz the power, 2016-06-07 03:11:06]

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

[commit-bot: I haz the power, 2016-06-07 04:10:44]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-06-07 04:12:20]

Description was changed from

==========
Limit the number of ARC codec

BUG=b/28157714
TEST=create too much ARC codecs and it should fail when reach limit
==========

to

==========
Limit the number of ARC codec

BUG=b/28157714
TEST=create too much ARC codecs and it should fail when reach limit

Committed: https://crrev.com/1b88e4a9efae89275f1ae2425768d39988d28e45
Cr-Commit-Position: refs/heads/master@{#398218}
==========

[commit-bot: I haz the power, 2016-06-07 04:12:21]

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