ppapi: define PPB_AudioEncoder API

ppapi: define PPB_AudioEncoder API

BUG=461222
Committed: https://crrev.com/33e90877de32c54a3e1e735f6fc314a8950c6cc0
Cr-Commit-Position: refs/heads/master@{#349418}

[llandwerlin-old, 2015-05-19 11:16:39]

lionel.g.landwerlin@intel.com changed reviewers:
+ bbudge@chromium.org

[llandwerlin-old, 2015-05-19 11:16:40]

Hi Bill,

Here is the boilerplate CL for the PPB_AudioeEncoder API I just sent the
proposal on Pepper-Dev.

[bbudge, 2015-05-26 23:20:23]

Do you have a clear idea of which AudioEncoder machinery in Chromium that you
are going to use for the implementation? That will help me review the API and
terminology.

[llandwerlin-old, 2015-05-27 09:10:00]

On 2015/05/26 23:20:23, bbudge wrote:
> Do you have a clear idea of which AudioEncoder machinery in Chromium that you
> are going to use for the implementation? That will help me review the API and
> terminology.

I had to write some specific for this :

https://codereview.chromium.org/1151973003/diff/1/content/renderer/pepper/aud...

(https://codereview.chromium.org/1151973003 is a possible implementation)

It's based on something already existing :

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/sp...

[llandwerlin-old, 2015-06-04 09:31:55]

Anything I can do to help with the review? Thanks for your time!

[bbudge, 2015-06-09 17:29:41]

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/pp_codecs.idl
File ppapi/api/pp_codecs.idl (right):

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/pp_codecs.idl...
ppapi/api/pp_codecs.idl:32: PP_AUDIOPROFILE_MAX = PP_AUDIOPROFILE_SPEEX
Is it possible to support AAC? This is very useful for developers, since they
can use Chrome's built-in support rather than deal with the licensing issues if
they build encoding into their plugin.

[llandwerlin-old, 2015-06-10 09:06:19]

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/pp_codecs.idl
File ppapi/api/pp_codecs.idl (right):

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/pp_codecs.idl...
ppapi/api/pp_codecs.idl:32: PP_AUDIOPROFILE_MAX = PP_AUDIOPROFILE_SPEEX
On 2015/06/09 17:29:41, bbudge wrote:
> Is it possible to support AAC? This is very useful for developers, since they
> can use Chrome's built-in support rather than deal with the licensing issues
if
> they build encoding into their plugin.

It's possible to support AAC, although it seems chromium does not embeds an AAC
encoder.
It relies on the system's libraries, the only platforms that support it are
MacOSX and Android.

[robert.bradford, 2015-08-17 09:48:55]

robert.bradford@intel.com changed reviewers:
+ robert.bradford@intel.com

[robert.bradford, 2015-08-17 09:48:56]

CC + abodenha@

[robert.bradford, 2015-08-17 10:37:49]

robert.bradford@intel.com changed reviewers:
- robert.bradford@intel.com

[Albert Bodenhamer, 2015-08-17 20:23:03]

On 2015/08/17 09:48:56, robert.bradford wrote:
> CC + abodenha@

My team (Chrome OS UI) would really like to see this added for Chrome OS. It's
valuable to us with or without AAC support.

Could we get this change landed in its current form and explore AAC as a
separate effort?

[chromium-reviews, 2015-08-18 22:28:43]

I'm willing to review the CL. We should get more input about whether we
want to land this from people on this thread.

-Bill

On Mon, Aug 17, 2015 at 1:23 PM, <abodenha@chromium.org> wrote:

> On 2015/08/17 09:48:56, robert.bradford wrote:
>
>> CC + abodenha@
>>
>
> My team (Chrome OS UI) would really like to see this added for Chrome OS.
> It's
> valuable to us with or without AAC support.
>
> Could we get this change landed in its current form and explore AAC as a
> separate effort?
>
> https://codereview.chromium.org/1128023009/
>

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

[Albert Bodenhamer, 2015-09-04 15:46:52]

What is standing in the way of reviewing this and landing it?

On 2015/08/18 22:28:43, chromium-reviews wrote:
> I'm willing to review the CL. We should get more input about whether we
> want to land this from people on this thread.
> 
> -Bill
> 
> On Mon, Aug 17, 2015 at 1:23 PM, <mailto:abodenha@chromium.org> wrote:
> 
> > On 2015/08/17 09:48:56, robert.bradford wrote:
> >
> >> CC + abodenha@
> >>
> >
> > My team (Chrome OS UI) would really like to see this added for Chrome OS.
> > It's
> > valuable to us with or without AAC support.
> >
> > Could we get this change landed in its current form and explore AAC as a
> > separate effort?
> >
> > https://codereview.chromium.org/1128023009/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[bbudge, 2015-09-04 16:26:51]

Sorry for the delay. LGTM w/nits

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/pp_codecs.idl
File ppapi/api/pp_codecs.idl (right):

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/pp_codecs.idl...
ppapi/api/pp_codecs.idl:32: PP_AUDIOPROFILE_MAX = PP_AUDIOPROFILE_SPEEX
On 2015/06/10 09:06:19, llandwerlin wrote:
> On 2015/06/09 17:29:41, bbudge wrote:
> > Is it possible to support AAC? This is very useful for developers, since
they
> > can use Chrome's built-in support rather than deal with the licensing issues
> if
> > they build encoding into their plugin.
> 
> It's possible to support AAC, although it seems chromium does not embeds an
AAC
> encoder.
> It relies on the system's libraries, the only platforms that support it are
> MacOSX and Android.

OK.

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/ppb_audio_enc...
File ppapi/api/ppb_audio_encoder.idl (right):

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/ppb_audio_enc...
ppapi/api/ppb_audio_encoder.idl:21: * - Call GetSupportedFormats() to determine
which codecs and profiles are
GetSupportedProfiles

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/ppb_audio_enc...
ppapi/api/ppb_audio_encoder.idl:24: * - Call GetBuffer() to get a blank frame
and fill it in, or get an audio
Frame is kind of confusing terminology here.
s/blank frame/empty buffer

and below.

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/ppb_audio_enc...
ppapi/api/ppb_audio_encoder.idl:113: * returned by GetBuffer().
nit: formatting

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/ppb_audio_enc...
ppapi/api/ppb_audio_encoder.idl:126: * encoder.
nit: this could fit on previous line.

[llandwerlin-old, 2015-09-04 17:39:05]

lionel.g.landwerlin@intel.com changed reviewers:
+ asvitkine@chromium.org, binji@chromium.org, thakis@chromium.org

[llandwerlin-old, 2015-09-04 17:39:05]

asvitkine@chromium.org: Could you review the changes to the histograms.xml file?
Thanks

thakis@chromium.org: Could you review the changes in chrome/*? Thanks

binji@chromium.org: Could you review the changes in native_client_sdk/*? Thanks.

[Alexei Svitkine (slow), 2015-09-04 17:46:51]

lgtm

[llandwerlin-old, 2015-09-07 09:37:25]

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/ppb_audio_enc...
File ppapi/api/ppb_audio_encoder.idl (right):

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/ppb_audio_enc...
ppapi/api/ppb_audio_encoder.idl:21: * - Call GetSupportedFormats() to determine
which codecs and profiles are
On 2015/09/04 16:26:50, bbudge wrote:
> GetSupportedProfiles

Done.

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/ppb_audio_enc...
ppapi/api/ppb_audio_encoder.idl:24: * - Call GetBuffer() to get a blank frame
and fill it in, or get an audio
On 2015/09/04 16:26:50, bbudge wrote:
> Frame is kind of confusing terminology here.
> s/blank frame/empty buffer
> 
> and below.

Done.

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/ppb_audio_enc...
ppapi/api/ppb_audio_encoder.idl:113: * returned by GetBuffer().
On 2015/09/04 16:26:50, bbudge wrote:
> nit: formatting

Done.

https://codereview.chromium.org/1128023009/diff/60001/ppapi/api/ppb_audio_enc...
ppapi/api/ppb_audio_encoder.idl:126: * encoder.
On 2015/09/04 16:26:50, bbudge wrote:
> nit: this could fit on previous line.

Done.

[llandwerlin-old, 2015-09-14 09:51:37]

binji@, thakis@: ping?

[binji, 2015-09-14 17:31:18]

On 2015/09/14 at 09:51:37, lionel.g.landwerlin wrote:
> binji@, thakis@: ping?

native_client_sdk lgtm

[Nico, 2015-09-15 02:56:51]

lgtm if the reason for the include in chrome/ is "macro magic", but in that case
some comment in that file should explain that too

https://codereview.chromium.org/1128023009/diff/80001/chrome/common/ppapi_uti...
File chrome/common/ppapi_utils.cc (right):

https://codereview.chromium.org/1128023009/diff/80001/chrome/common/ppapi_uti...
chrome/common/ppapi_utils.cc:33: #include "ppapi/c/ppb_audio_encoder.h"
Is this needed? There's no code change in this file other than the addition of
the header.

[llandwerlin-old, 2015-09-15 09:09:22]

Patchset #6 (id:100001) has been deleted

[llandwerlin-old, 2015-09-15 09:27:56]

https://codereview.chromium.org/1128023009/diff/80001/chrome/common/ppapi_uti...
File chrome/common/ppapi_utils.cc (right):

https://codereview.chromium.org/1128023009/diff/80001/chrome/common/ppapi_uti...
chrome/common/ppapi_utils.cc:33: #include "ppapi/c/ppb_audio_encoder.h"
On 2015/09/15 02:56:51, Nico wrote:
> Is this needed? There's no code change in this file other than the addition of
> the header.

This is needed because of 
PROXIED_IFACE(PPB_AUDIOENCODER_INTERFACE_0_1, PPB_AudioEncoder_0_1)

in ppapi/thunk/interfaces_ppb_public_dev_channel.h.

This is macro magic to figure what interfaces are supported.
I'm not sure what comment I should add in files using this trick.

[llandwerlin-old, 2015-09-17 13:18:30]

The CQ bit was checked by lionel.g.landwerlin@intel.com

[llandwerlin-old, 2015-09-17 13:18:31]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1128023009/#ps80001
(title: "bbudge's nits & api release bump")

[commit-bot: I haz the power, 2015-09-17 13:18:50]

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

[commit-bot: I haz the power, 2015-09-17 14:41:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-17 14:42:13]

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