Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1348563003:
ppapi: implement PPB_AudioEncoder (Closed)
|
Created:
5 years, 3 months ago by llandwerlin-old Modified:
5 years, 1 month ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionppapi: implement PPB_AudioEncoder
BUG=461222
Committed: https://crrev.com/764a8c9512e79b62621e997aa802a6bcfd2c171c
Cr-Commit-Position: refs/heads/master@{#360469}
Patch Set 1 : #
Total comments: 54
Patch Set 2 : Address tsepez's review #
Total comments: 8
Patch Set 3 : Address remaining overflow issues #
Total comments: 6
Patch Set 4 : Remove some internal classes #Patch Set 5 : Add ppapi tests #Patch Set 6 : Drop checks on buffers already queued #
Total comments: 4
Patch Set 7 : Remove AudioEncoderShim #Patch Set 8 : Fold internal classes into AudioEncoderImpl #
Total comments: 15
Patch Set 9 : Switch to int32_t buffer ids #
Total comments: 2
Patch Set 10 : Rework destruction sequence #
Total comments: 20
Patch Set 11 : Rework allocation methods #
Total comments: 14
Patch Set 12 : Drop bitstream buffer manager from proxy #
Total comments: 62
Patch Set 13 : Add bitstream buffer type to MediaStreamBuffer #
Total comments: 19
Patch Set 14 : Don't rely on sizes from the plugin #
Total comments: 12
Patch Set 15 : #
Total comments: 1
Messages
Total messages: 69 (19 generated)
Patchset #1 (id:1) has been deleted
lionel.g.landwerlin@intel.com changed reviewers: + bbudge@chromium.org, tsepez@chromium.org
tsepez@chromium.org: Could you review the ppapi_messages.h changes? bbudge@chromium.org: Could you review the content/renderer/pepper and ppapi/proxy changes? I did have a Speex support in a previous iteration of this CL, but the dependency has been dropped about a month ago. Thanks.
The messages themselves are fine, but I worry about the implementation behind them. How does the code cope with messages from an untrusted renderer sending the worst possible values? https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... File content/renderer/pepper/audio_encoder_shim.cc (right): https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.cc:99: uint32_t sampling_rates[] = {8000, 12000, 16000, 24000, 48000}; nit: probably a static const. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.cc:101: for (uint32_t i = 0; i < arraysize(sampling_rates); i++) { nit: ++i https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.cc:179: for (const PP_AudioProfileDescription desc : codec_profiles) nit: how about const auto& desc : https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.cc:190: encoder_impl_.reset(new OpusEncoderImpl(weak_ptr_factory_.GetWeakPtr())); nit: prefer early return if !=, combine with line 186. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.cc:197: DCHECK(encoder_impl_); nit: pointless DCHECK, the next line will segv if the check fails, so we don't need extra instructions in the program to catch this. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.cc:220: int32_t size, nit: sizes should usually be size_t's unless you have a good reason otherwise. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... File content/renderer/pepper/audio_encoder_shim.h (right): https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.h:34: virtual uint8_t* GetData() = 0; nit: indentation. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.h:49: typedef base::Callback<void(const scoped_refptr<AudioData>& output, nit: using. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:36: const uint32_t kDefaultBitstreamBufferSize = 100000; nit: size as size_t unless good reason otherwise? https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:101: ->HostAllocateSharedMemoryBuffer(number_of_buffers * How do we know this multiplication doesn't overflow? https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:109: for (int32_t i = 0; i < base::checked_cast<int32_t>(number_of_buffers); Why the int and the checked cast rather than just size_t i? https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:110: i++) { nit: ++i https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:151: size_t total_size = number_of_buffers * buffer_size; again, is overflow possible? or do we control these values in such a way that its not? https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:164: return static_cast<uint8_t*>(shm_->memory()) + (id * buffer_size()); wouldn't an untrusted ID value lead to an arbitrary memory read? https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:173: void EnqueueBuffer(int32_t id) override { enqueued_buffers_.push_back(id); } Are duplicate IDs in the queue allowed? https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:304: // audio_buffer_manager_->EnqueueBuffer(buffer_id); nit: remove dead code, dont just // it. Why are you violating your abstraction and pushing directly? https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:387: uint32_t buffer_size = number_of_samples * encode_parameters_.channels * worry about overflow here, too. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... File content/renderer/pepper/pepper_audio_encoder_host.h (right): https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.h:83: // Last error reported by the encoder. nit: as defined where? Is there an enum type representing the error codes or a bunch of #defines in some header, for example? https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.h:90: std::deque<int32_t> pending_encode_buffers_; nit: is there a reason IDs are signed? https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... File ppapi/proxy/audio_encoder_resource.cc (right): https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.cc:60: return static_cast<uint8_t*>(shm_->memory()) + (id * buffer_size_); I'd be happier if we did the bounds checking here, returning NULL, rather than relying on callers. This becomes an arbitrary memory read primitive if we can't trust the ID. https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.cc:78: uint32_t buffer_size_; Should be size_t. Comment should say "in bytes" otherwise is it samples, etc? https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.cc:80: // Shared memory containing all objects. nit: can you be more specific than "all objects"? https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.cc:88: typedef std::map<void*, int32_t> BufferMap; nit: using https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.cc:155: if (encoder_last_error_) nit: really dislike overloading the error return and value. It relies on some quirk of the last_error value making it distinguishable by the caller from the valid numbers.
Sorry, for the delay in addressing your comments. bbudge@: Could you comment on tsepez's point about GetNumberOfSamples()? Thanks https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... File content/renderer/pepper/audio_encoder_shim.cc (right): https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.cc:99: uint32_t sampling_rates[] = {8000, 12000, 16000, 24000, 48000}; On 2015/09/17 18:11:12, Tom Sepez wrote: > nit: probably a static const. Done. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.cc:101: for (uint32_t i = 0; i < arraysize(sampling_rates); i++) { On 2015/09/17 18:11:12, Tom Sepez wrote: > nit: ++i Done. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.cc:179: for (const PP_AudioProfileDescription desc : codec_profiles) On 2015/09/17 18:11:12, Tom Sepez wrote: > nit: how about const auto& desc : Done. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.cc:190: encoder_impl_.reset(new OpusEncoderImpl(weak_ptr_factory_.GetWeakPtr())); On 2015/09/17 18:11:12, Tom Sepez wrote: > nit: prefer early return if !=, combine with line 186. Done. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.cc:197: DCHECK(encoder_impl_); On 2015/09/17 18:11:12, Tom Sepez wrote: > nit: pointless DCHECK, the next line will segv if the check fails, so we don't > need extra instructions in the program to catch this. Done. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.cc:220: int32_t size, On 2015/09/17 18:11:12, Tom Sepez wrote: > nit: sizes should usually be size_t's unless you have a good reason otherwise. Done. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... File content/renderer/pepper/audio_encoder_shim.h (right): https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.h:34: virtual uint8_t* GetData() = 0; On 2015/09/17 18:11:12, Tom Sepez wrote: > nit: indentation. Done. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.h:49: typedef base::Callback<void(const scoped_refptr<AudioData>& output, On 2015/09/17 18:11:12, Tom Sepez wrote: > nit: using. Done. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:36: const uint32_t kDefaultBitstreamBufferSize = 100000; On 2015/09/17 18:11:13, Tom Sepez wrote: > nit: size as size_t unless good reason otherwise? Done. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:101: ->HostAllocateSharedMemoryBuffer(number_of_buffers * On 2015/09/17 18:11:13, Tom Sepez wrote: > How do we know this multiplication doesn't overflow? Added CheckedNumeric. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:109: for (int32_t i = 0; i < base::checked_cast<int32_t>(number_of_buffers); On 2015/09/17 18:11:13, Tom Sepez wrote: > Why the int and the checked cast rather than just size_t i? Switched to uint32_t i, because number_of_buffers is uint32_t too. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:110: i++) { On 2015/09/17 18:11:12, Tom Sepez wrote: > nit: ++i Done. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:151: size_t total_size = number_of_buffers * buffer_size; On 2015/09/17 18:11:12, Tom Sepez wrote: > again, is overflow possible? or do we control these values in such a way that > its not? Thanks, using CheckedNumeric. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:164: return static_cast<uint8_t*>(shm_->memory()) + (id * buffer_size()); On 2015/09/17 18:11:13, Tom Sepez wrote: > wouldn't an untrusted ID value lead to an arbitrary memory read? Thanks, adding a check against the number of buffers. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:173: void EnqueueBuffer(int32_t id) override { enqueued_buffers_.push_back(id); } On 2015/09/17 18:11:12, Tom Sepez wrote: > Are duplicate IDs in the queue allowed? Thanks, adding checks. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:304: // audio_buffer_manager_->EnqueueBuffer(buffer_id); On 2015/09/17 18:11:13, Tom Sepez wrote: > nit: remove dead code, dont just // it. Why are you violating your abstraction > and pushing directly? Whoops, removing the pending_encode_buffers_ completely. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:387: uint32_t buffer_size = number_of_samples * encode_parameters_.channels * On 2015/09/17 18:11:13, Tom Sepez wrote: > worry about overflow here, too. Using CheckedNumeric here too. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... File content/renderer/pepper/pepper_audio_encoder_host.h (right): https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.h:83: // Last error reported by the encoder. On 2015/09/17 18:11:13, Tom Sepez wrote: > nit: as defined where? Is there an enum type representing the error codes or a > bunch of #defines in some header, for example? It's a pepper error code, added a comment. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.h:90: std::deque<int32_t> pending_encode_buffers_; On 2015/09/17 18:11:13, Tom Sepez wrote: > nit: is there a reason IDs are signed? Done. https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... File ppapi/proxy/audio_encoder_resource.cc (right): https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.cc:60: return static_cast<uint8_t*>(shm_->memory()) + (id * buffer_size_); On 2015/09/17 18:11:13, Tom Sepez wrote: > I'd be happier if we did the bounds checking here, returning NULL, rather than > relying on callers. This becomes an arbitrary memory read primitive if we > can't trust the ID. Done. https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.cc:78: uint32_t buffer_size_; On 2015/09/17 18:11:13, Tom Sepez wrote: > Should be size_t. Comment should say "in bytes" otherwise is it samples, etc? Done. https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.cc:80: // Shared memory containing all objects. On 2015/09/17 18:11:13, Tom Sepez wrote: > nit: can you be more specific than "all objects"? Done. https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.cc:88: typedef std::map<void*, int32_t> BufferMap; On 2015/09/17 18:11:13, Tom Sepez wrote: > nit: using Done. https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.cc:155: if (encoder_last_error_) On 2015/09/17 18:11:13, Tom Sepez wrote: > nit: really dislike overloading the error return and value. It relies on some > quirk of the last_error value making it distinguishable by the caller from the > valid numbers. Sadly this is how the API works. It would require changing the IDL. This is a pattern already using in the VideoEncoder (I'm responsible for that too). bbudge: What's your opinion on changing the API?
https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... File ppapi/proxy/audio_encoder_resource.cc (right): https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.cc:155: if (encoder_last_error_) On 2015/10/05 16:11:00, llandwerlin wrote: > On 2015/09/17 18:11:13, Tom Sepez wrote: > > nit: really dislike overloading the error return and value. It relies on some > > quirk of the last_error value making it distinguishable by the caller from the > > valid numbers. > > Sadly this is how the API works. It would require changing the IDL. > This is a pattern already using in the VideoEncoder (I'm responsible for that > too). > > bbudge: What's your opinion on changing the API? Pepper is a little inconsistent in how functions like this work, but there are precedents for this style. The most obvious one is PPB_FileIO Read, which returns the number of bytes read. Values > 0 indicate that bytes were read, == 0 means EOF, and < 0 is an error code from pp_errors.h. https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file... Other precedents are PPB_VideoEncoder and PPB_TrueTypeFont (so I suppose I'm responsible for a lot of the precedents either directly or indirectly). If we did want to change this, we should probably also change PPB_VideoEncoder to be consistent.
Getting Closer. https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... File content/renderer/pepper/audio_encoder_shim.h (right): https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.h:49: typedef base::Callback<void(const scoped_refptr<AudioData>& output, On 2015/10/05 16:10:59, llandwerlin wrote: > On 2015/09/17 18:11:12, Tom Sepez wrote: > > nit: using. > > Done. No :), I meant that our style guide prefers |using| vs |typedef|, eg. using BitStreamBufferReadyCB = base::Callback<...>; No worries, we can let this be for now. https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... File ppapi/proxy/audio_encoder_resource.cc (right): https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.cc:88: typedef std::map<void*, int32_t> BufferMap; On 2015/09/17 18:11:13, Tom Sepez wrote: > nit: using using BufferMap = std::map<void*, int32_t>; https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.cc:155: if (encoder_last_error_) On 2015/10/05 16:34:08, bbudge wrote: > On 2015/10/05 16:11:00, llandwerlin wrote: > > On 2015/09/17 18:11:13, Tom Sepez wrote: > > > nit: really dislike overloading the error return and value. It relies on > some > > > quirk of the last_error value making it distinguishable by the caller from > the > > > valid numbers. > > > > Sadly this is how the API works. It would require changing the IDL. > > This is a pattern already using in the VideoEncoder (I'm responsible for that > > too). > > > > bbudge: What's your opinion on changing the API? > > Pepper is a little inconsistent in how functions like this work, but there are > precedents for this style. The most obvious one is PPB_FileIO Read, which > returns the number of bytes read. Values > 0 indicate that bytes were read, == 0 > means EOF, and < 0 is an error code from pp_errors.h. > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file... > > Other precedents are PPB_VideoEncoder and PPB_TrueTypeFont (so I suppose I'm > responsible for a lot of the precedents either directly or indirectly). > > If we did want to change this, we should probably also change PPB_VideoEncoder > to be consistent. So it goes. Let's leave it. https://codereview.chromium.org/1348563003/diff/40001/content/renderer/pepper... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:118: shm_ = RenderThreadImpl::current() you might want to call IsValid() and return false rather than dying. It looks like we return false in other conditions like failed shm. https://codereview.chromium.org/1348563003/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:165: base::CheckedNumeric<size_t> total_size = number_of_buffers * buffer_size; need to assign first, then multiply as you did above. What this currently does is to first do an unsafe multiplication, and then assign the possibly overflowed result to the safe type. https://codereview.chromium.org/1348563003/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:396: encode_parameters_.channels * encode_parameters_.input_sample_size; again, you need to do the multplications one at a time against buffer_size, unless were positive that the unsafe multiplication encode_parameters_.channels * encode_parameters_.input_sample_size can't overflow before being promoted to the safe type https://codereview.chromium.org/1348563003/diff/40001/content/renderer/pepper... File content/renderer/pepper/pepper_audio_encoder_host.h (right): https://codereview.chromium.org/1348563003/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.h:87: // other unrecoverable error, or has been closed by the plugin. nit: extra space between "by" and "the"
https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... File content/renderer/pepper/audio_encoder_shim.h (right): https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... content/renderer/pepper/audio_encoder_shim.h:49: typedef base::Callback<void(const scoped_refptr<AudioData>& output, On 2015/10/05 16:54:16, Tom Sepez wrote: > On 2015/10/05 16:10:59, llandwerlin wrote: > > On 2015/09/17 18:11:12, Tom Sepez wrote: > > > nit: using. > > > > Done. > No :), I meant that our style guide prefers |using| vs |typedef|, eg. > > using BitStreamBufferReadyCB = base::Callback<...>; > > No worries, we can let this be for now. Right, I was a bit puzzled with this nit, makes sense now :) https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... File ppapi/proxy/audio_encoder_resource.cc (right): https://codereview.chromium.org/1348563003/diff/20001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.cc:88: typedef std::map<void*, int32_t> BufferMap; On 2015/10/05 16:54:16, Tom Sepez wrote: > On 2015/09/17 18:11:13, Tom Sepez wrote: > > nit: using > using BufferMap = std::map<void*, int32_t>; Done. https://codereview.chromium.org/1348563003/diff/40001/content/renderer/pepper... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:118: shm_ = RenderThreadImpl::current() On 2015/10/05 16:54:16, Tom Sepez wrote: > you might want to call IsValid() and return false rather than dying. It looks > like we return false in other conditions like failed shm. Done. https://codereview.chromium.org/1348563003/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:165: base::CheckedNumeric<size_t> total_size = number_of_buffers * buffer_size; On 2015/10/05 16:54:16, Tom Sepez wrote: > need to assign first, then multiply as you did above. What this currently does > is to first do an unsafe multiplication, and then assign the possibly overflowed > result to the safe type. Sorry, missed this one. https://codereview.chromium.org/1348563003/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.cc:396: encode_parameters_.channels * encode_parameters_.input_sample_size; On 2015/10/05 16:54:16, Tom Sepez wrote: > again, you need to do the multplications one at a time against buffer_size, > unless were positive that the unsafe multiplication > encode_parameters_.channels * encode_parameters_.input_sample_size > can't overflow before being promoted to the safe type Ok, left this one because the encode_parameters_ are supposed to be verified in OnHostMsgInitialize(), but you're right. https://codereview.chromium.org/1348563003/diff/40001/content/renderer/pepper... File content/renderer/pepper/pepper_audio_encoder_host.h (right): https://codereview.chromium.org/1348563003/diff/40001/content/renderer/pepper... content/renderer/pepper/pepper_audio_encoder_host.h:87: // other unrecoverable error, or has been closed by the plugin. On 2015/10/05 16:54:16, Tom Sepez wrote: > nit: extra space between "by" and "the" Done.
lgtm
Some high level comments. https://codereview.chromium.org/1348563003/diff/60001/content/renderer/pepper... File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right): https://codereview.chromium.org/1348563003/diff/60001/content/renderer/pepper... content/renderer/pepper/content_renderer_pepper_host_factory.cc:119: case PpapiHostMsg_AudioEncoder_Create::ID: This should be handled with the other 'Dev' APIs below. https://codereview.chromium.org/1348563003/diff/60001/ppapi/proxy/audio_encod... File ppapi/proxy/audio_encoder_resource.h (right): https://codereview.chromium.org/1348563003/diff/60001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.h:42: // PPB_AduioEncoder_API implementation. nit: PPB_AudioEncoder_API https://codereview.chromium.org/1348563003/diff/60001/ppapi/proxy/ppapi_messa... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1348563003/diff/60001/ppapi/proxy/ppapi_messa... ppapi/proxy/ppapi_messages.h:921: IPC_MESSAGE_CONTROL0(PpapiHostMsg_AudioEncoder_GetAudioFrames) The IDL for PPB_AudioEncoder talks about audio buffers and bitstream buffers, not frames. Could we use a consistent terminology here too?
lionel.g.landwerlin@intel.com changed reviewers: + kinuko@chromium.org
kinuko@chromium.org: Could you review the content/renderer/BUILD.gn changes? Thanks.
https://codereview.chromium.org/1348563003/diff/60001/content/renderer/pepper... File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right): https://codereview.chromium.org/1348563003/diff/60001/content/renderer/pepper... content/renderer/pepper/content_renderer_pepper_host_factory.cc:119: case PpapiHostMsg_AudioEncoder_Create::ID: On 2015/10/06 00:54:25, bbudge wrote: > This should be handled with the other 'Dev' APIs below. Done. https://codereview.chromium.org/1348563003/diff/60001/ppapi/proxy/audio_encod... File ppapi/proxy/audio_encoder_resource.h (right): https://codereview.chromium.org/1348563003/diff/60001/ppapi/proxy/audio_encod... ppapi/proxy/audio_encoder_resource.h:42: // PPB_AduioEncoder_API implementation. On 2015/10/06 00:54:25, bbudge wrote: > nit: PPB_AudioEncoder_API Done. https://codereview.chromium.org/1348563003/diff/60001/ppapi/proxy/ppapi_messa... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1348563003/diff/60001/ppapi/proxy/ppapi_messa... ppapi/proxy/ppapi_messages.h:921: IPC_MESSAGE_CONTROL0(PpapiHostMsg_AudioEncoder_GetAudioFrames) On 2015/10/06 00:54:25, bbudge wrote: > The IDL for PPB_AudioEncoder talks about audio buffers and bitstream buffers, > not frames. Could we use a consistent terminology here too? Done.
Patchset #5 (id:100001) has been deleted
More high level comments. https://codereview.chromium.org/1348563003/diff/120001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/120001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:54: class PepperAudioEncoderHost::BufferManager This looks similar to ppapi::MediaStreamBufferManager. Is there a reason you can't use that instead of defining this base class? https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/... I also don't see why these buffer managers need to be ref-counted. https://codereview.chromium.org/1348563003/diff/120001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:100: class PepperAudioEncoderHost::PcmBufferManager : public BufferManager { These derived classes exist mostly to specialize Initialize and GetBuffer. You could just move this code to the host class. It doesn't seem worth the extra complexity to have this class hierarchy.
https://codereview.chromium.org/1348563003/diff/120001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/120001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:54: class PepperAudioEncoderHost::BufferManager On 2015/10/06 20:07:32, bbudge wrote: > This looks similar to ppapi::MediaStreamBufferManager. Is there a reason you > can't use that instead of defining this base class? > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/... > > I also don't see why these buffer managers need to be ref-counted. Yes, there is no method on MediaStreamBufferManager to know how many buffers/if any buffer is queued, or if a particular buffer is queued. I remember submitting a CL to add the first method, but it didn't make through the review. The BufferManagers are ref-counted because they're used from 2 different threads (media for the encoding and renderer for talking to plugin). https://codereview.chromium.org/1348563003/diff/120001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:100: class PepperAudioEncoderHost::PcmBufferManager : public BufferManager { On 2015/10/06 20:07:32, bbudge wrote: > These derived classes exist mostly to specialize Initialize and GetBuffer. You > could just move this code to the host class. It doesn't seem worth the extra > complexity to have this class hierarchy. Removed the subclasses, ~20 fewer lines.
Ping?
BUILD.gn change lgtm
Sorry for the delay. https://codereview.chromium.org/1348563003/diff/120001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/120001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:54: class PepperAudioEncoderHost::BufferManager On 2015/10/07 14:04:19, llandwerlin wrote: > On 2015/10/06 20:07:32, bbudge wrote: > > This looks similar to ppapi::MediaStreamBufferManager. Is there a reason you > > can't use that instead of defining this base class? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/... > > > > I also don't see why these buffer managers need to be ref-counted. > > Yes, there is no method on MediaStreamBufferManager to know how many buffers/if > any buffer is queued, or if a particular buffer is queued. > I remember submitting a CL to add the first method, but it didn't make through > the review. > > The BufferManagers are ref-counted because they're used from 2 different threads > (media for the encoding and renderer for talking to plugin). Can you give me a link to that review? It doesn't make sense to duplicate the class to get around that.
On 2015/10/19 15:33:14, bbudge wrote: > Sorry for the delay. > > https://codereview.chromium.org/1348563003/diff/120001/content/renderer/peppe... > File content/renderer/pepper/pepper_audio_encoder_host.cc (right): > > https://codereview.chromium.org/1348563003/diff/120001/content/renderer/peppe... > content/renderer/pepper/pepper_audio_encoder_host.cc:54: class > PepperAudioEncoderHost::BufferManager > On 2015/10/07 14:04:19, llandwerlin wrote: > > On 2015/10/06 20:07:32, bbudge wrote: > > > This looks similar to ppapi::MediaStreamBufferManager. Is there a reason you > > > can't use that instead of defining this base class? > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/... > > > > > > I also don't see why these buffer managers need to be ref-counted. > > > > Yes, there is no method on MediaStreamBufferManager to know how many > buffers/if > > any buffer is queued, or if a particular buffer is queued. > > I remember submitting a CL to add the first method, but it didn't make through > > the review. > > > > The BufferManagers are ref-counted because they're used from 2 different > threads > > (media for the encoding and renderer for talking to plugin). > > Can you give me a link to that review? It doesn't make sense to duplicate the > class to get around that. I think it was in reaction to this comment : https://codereview.chromium.org/884723002/#msg4 At the time I wanted to add these methods : https://codereview.chromium.org/884723002/diff/1/ppapi/shared_impl/media_stre... I was under the impression that because this is shared code (ending up in different binaries), we needed to be careful not to add too many methods.
On 2015/10/19 16:38:31, llandwerlin wrote: > On 2015/10/19 15:33:14, bbudge wrote: > > Sorry for the delay. > > > > > https://codereview.chromium.org/1348563003/diff/120001/content/renderer/peppe... > > File content/renderer/pepper/pepper_audio_encoder_host.cc (right): > > > > > https://codereview.chromium.org/1348563003/diff/120001/content/renderer/peppe... > > content/renderer/pepper/pepper_audio_encoder_host.cc:54: class > > PepperAudioEncoderHost::BufferManager > > On 2015/10/07 14:04:19, llandwerlin wrote: > > > On 2015/10/06 20:07:32, bbudge wrote: > > > > This looks similar to ppapi::MediaStreamBufferManager. Is there a reason > you > > > > can't use that instead of defining this base class? > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/... > > > > > > > > I also don't see why these buffer managers need to be ref-counted. > > > > > > Yes, there is no method on MediaStreamBufferManager to know how many > > buffers/if > > > any buffer is queued, or if a particular buffer is queued. > > > I remember submitting a CL to add the first method, but it didn't make > through > > > the review. > > > > > > The BufferManagers are ref-counted because they're used from 2 different > > threads > > > (media for the encoding and renderer for talking to plugin). > > > > Can you give me a link to that review? It doesn't make sense to duplicate the > > class to get around that. > > I think it was in reaction to this comment : > https://codereview.chromium.org/884723002/#msg4 > At the time I wanted to add these methods : > https://codereview.chromium.org/884723002/diff/1/ppapi/shared_impl/media_stre... > > I was under the impression that because this is shared code (ending up in > different binaries), we needed to be careful not to add too many methods. On that CL, I was only worrying about breaking the encapsulation of some internal data. As long as your changes don't make the class hazardous to use, I think it's OK. If you like, we can split this into a separate CL. It looks like you only need a new method to determine if a give buffer is currently enqueued.
bbudge@: PTAL, thanks.
Some small comments and one design one. The video APIs were designed for hardware decode / encode using the hardware APIs that Chrome defines. We added software fallback support, necessitating shim classes to make software APIs match the hardware APIs. This CL defines a host that only works with the AudioEncoderShim class. If there is no hardware audio encode API, then this doesn't make sense, and you can combine these two classes. The host class is very dependent on the shim class so separating them isn't useful. On a related note, I noticed that Chrome does have AAC support: https://www.chromium.org/audio-video Is the reason we can't support AAC here that only decode is supported? https://codereview.chromium.org/1348563003/diff/180001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.h (right): https://codereview.chromium.org/1348563003/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:8: #include <deque> Is this needed now? https://codereview.chromium.org/1348563003/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:39: class PcmBufferManager; Not defined any more.
https://codereview.chromium.org/1348563003/diff/180001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.h (right): https://codereview.chromium.org/1348563003/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:8: #include <deque> On 2015/10/23 23:18:41, bbudge wrote: > Is this needed now? Removing. https://codereview.chromium.org/1348563003/diff/180001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:39: class PcmBufferManager; On 2015/10/23 23:18:41, bbudge wrote: > Not defined any more. Done.
On 2015/10/23 23:18:41, bbudge wrote: > > On a related note, I noticed that Chrome does have AAC support: > > https://www.chromium.org/audio-video > > Is the reason we can't support AAC here that only decode is supported? > I think this is a list of supported codecs for decoding only.
High level suggestion: Collapse your AudioCodecBase / OpusEncoder class hierarchy into AudioEncoderImpl. Unless you know for sure that we're going to have other encoders. https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:27: // Number of input audio buffers (150ms at 10ms per frame). Suggestion: // Buffer up to 150ms (15 x 10ms per frame.) https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:30: // Number of bitstream buffers. Could this comment explain why 8? https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:33: // Default bitstream buffers size. nit:s/buffers/buffer https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:36: const size_t kDefaultBitstreamBufferSize = 100000; Seems very large to me but I'm not very knowledgable about codecs. Doesn't encode compress the data? https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:49: using BitstreamBufferReadyCB = base::Callback<void(int64_t size)>; s/int64_t/size_t https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:92: class AudioCodecBase { Are there other codecs besides opus that you know could reuse this abstract base class? Otherwise, I'd collapse this into a single class for now. https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:115: virtual int64_t EncodeInternal(const AudioData& input, Looks like int64_t comes from the opus_encode function, but that seems to return an int32. Any reason for upcasting to int64 here? https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:232: class PepperAudioEncoderHost::AudioEncoderImpl { Since you have this envelope class, why not make it your encoder (rather than AudioCodecBase)? And if you don't need the abstract base class yet, just collapse OpusEncoder into it for now. https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.h (right): https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:87: // Buffer manager for the audio buffers handed by the plugin. Suggestion for better comment: // Manages buffers containing audio samples from the plugin. https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:90: // Buffer manager for bitstream buffers handed to the plugin. // Manages buffers containing encoded audio from the browser. https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:93: // The encoder actually doing the work. This need to be after Good comment. nit s/need/needs
https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:27: // Number of input audio buffers (150ms at 10ms per frame). On 2015/11/02 21:15:56, bbudge wrote: > Suggestion: > // Buffer up to 150ms (15 x 10ms per frame.) Done. https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:30: // Number of bitstream buffers. On 2015/11/02 21:15:57, bbudge wrote: > Could this comment explain why 8? Using the same number as the input buffers. https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:33: // Default bitstream buffers size. On 2015/11/02 21:15:57, bbudge wrote: > nit:s/buffers/buffer Done. https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:36: const size_t kDefaultBitstreamBufferSize = 100000; On 2015/11/02 21:15:56, bbudge wrote: > Seems very large to me but I'm not very knowledgable about codecs. Doesn't > encode compress the data? Setting it to twice the size of the audio input buffer size. https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:49: using BitstreamBufferReadyCB = base::Callback<void(int64_t size)>; On 2015/11/02 21:15:57, bbudge wrote: > s/int64_t/size_t I would like to this value to signal potential errors with negative values. Would you prefer an int32_t or is there something problematic with that approach? https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:92: class AudioCodecBase { On 2015/11/02 21:15:56, bbudge wrote: > Are there other codecs besides opus that you know could reuse this abstract base > class? Otherwise, I'd collapse this into a single class for now. Done. https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:115: virtual int64_t EncodeInternal(const AudioData& input, On 2015/11/02 21:15:57, bbudge wrote: > Looks like int64_t comes from the opus_encode function, but that seems to return > an int32. Any reason for upcasting to int64 here? Switching to int32_t. https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:232: class PepperAudioEncoderHost::AudioEncoderImpl { On 2015/11/02 21:15:56, bbudge wrote: > Since you have this envelope class, why not make it your encoder (rather than > AudioCodecBase)? > > And if you don't need the abstract base class yet, just collapse OpusEncoder > into it for now. Done. https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.h (right): https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:87: // Buffer manager for the audio buffers handed by the plugin. On 2015/11/02 21:15:57, bbudge wrote: > Suggestion for better comment: > // Manages buffers containing audio samples from the plugin. Done. https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:90: // Buffer manager for bitstream buffers handed to the plugin. On 2015/11/02 21:15:57, bbudge wrote: > // Manages buffers containing encoded audio from the browser. Done. https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:93: // The encoder actually doing the work. This need to be after On 2015/11/02 21:15:57, bbudge wrote: > Good comment. nit s/need/needs Done.
Mostly comments about the host. https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/220001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:49: using BitstreamBufferReadyCB = base::Callback<void(int64_t size)>; On 2015/11/03 18:43:11, llandwerlin OOO until 1st wrote: > On 2015/11/02 21:15:57, bbudge wrote: > > s/int64_t/size_t > > I would like to this value to signal potential errors with negative values. > Would you prefer an int32_t or is there something problematic with that > approach? My mistake. I meant int32_t. https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:30: // Class used to pass audio data between the different threads. nit: s/the different// https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:78: AudioData GetBitstreamDataFromAudioBufferManager( s/GetBitstreamDataFromAudioBufferManager/GetBitstreamDataFromBitstreamBufferManager ? Or just GetBitstreamDataFromBufferManager. In that case change the preceding function to match. https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:313: if (encoder_->Initialize(parameters)) { It's unfortunate that you validate 'parameters' after you've passed them to the encoder. Opus seems to be safe, but chrome internal code isn't generally meant to deal with input from potentially malicious code. Could you do all the valdiation earlier in this method, using some upper bound on encoder_->GetNumberOfSamplesPerFrame()? An large upper bound seems feasible since with a size_t, reasonable parameters shouldn't be close to overflowing. I'm imagining you could move all your safe math into this method, and store the buffer and memory size results in member fields. On overflows, just return failed rather than crashing the renderer process. This would also allow you to eliminate the GetAudioBufferSize method. I think that would be clearer, and more robust than scattering the checks in different methods, which execute at different points in the lifetime of the host. https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:350: You should probably detect when this has been called twice here. AllocateAudioBuffers does this but the plugin receives PP_ERROR_NOMEMORY which is misleading. The error in the case of double allocation should be PP_ERROR_FAILED. https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:457: return false; remove this check (see comment at call site above.) https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:498: bool PepperAudioEncoderHost::AllocateBitstreamBuffers(size_t buffer_size) { If you follow some of my other suggestions for the host, I think this method shrinks to the point where you can just inline it in Initialize. https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.h (right): https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:28: class AudioEncoderImpl; Make this private. https://codereview.chromium.org/1348563003/diff/240001/ppapi/proxy/audio_enco... File ppapi/proxy/audio_encoder_resource.h (right): https://codereview.chromium.org/1348563003/diff/240001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.h:119: MediaStreamBufferManager buffer_manager_; s/buffer_manager_/audio_buffer_manager_ for consistency in this class and in the host.
I had the shutdown of the AudioEncoderImpl slightly wrong, it's now using a weak pointer for the callbacks. https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:30: // Class used to pass audio data between the different threads. On 2015/11/04 00:59:59, bbudge wrote: > nit: s/the different// Done. https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:78: AudioData GetBitstreamDataFromAudioBufferManager( On 2015/11/04 00:59:59, bbudge wrote: > s/GetBitstreamDataFromAudioBufferManager/GetBitstreamDataFromBitstreamBufferManager > ? > > Or just GetBitstreamDataFromBufferManager. In that case change the preceding > function to match. Done. https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:313: if (encoder_->Initialize(parameters)) { On 2015/11/04 00:59:59, bbudge wrote: > It's unfortunate that you validate 'parameters' after you've passed them to the > encoder. Opus seems to be safe, but chrome internal code isn't generally meant > to deal with input from potentially malicious code. Could you do all the > valdiation earlier in this method, using some upper bound on > encoder_->GetNumberOfSamplesPerFrame()? An large upper bound seems feasible > since with a size_t, reasonable parameters shouldn't be close to overflowing. The parameters are validated above line 304, those is just additional checks. I put them to be consistent with the input buffers allocation. Following your argument, one could say that safe math isn't useful for input buffers allocation either, because the size of the buffers is based upon validated parameters in this method and the value returned by GetNumberOfSamplesPerFrame() which is owned by host. > > I'm imagining you could move all your safe math into this method, and store the > buffer and memory size results in member fields. On overflows, just return > failed rather than crashing the renderer process. This would also allow you to > eliminate the GetAudioBufferSize method. I think that would be clearer, and more > robust than scattering the checks in different methods, which execute at > different points in the lifetime of the host. I just realized I can remove the parameters of the Allocate* methods because all of them can be inferred from the initialization parameters. https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:350: On 2015/11/04 00:59:59, bbudge wrote: > You should probably detect when this has been called twice here. > AllocateAudioBuffers does this but the plugin receives PP_ERROR_NOMEMORY which > is misleading. The error in the case of double allocation should be > PP_ERROR_FAILED. Done. https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:457: return false; On 2015/11/04 00:59:59, bbudge wrote: > remove this check (see comment at call site above.) Done. https://codereview.chromium.org/1348563003/diff/240001/ppapi/proxy/audio_enco... File ppapi/proxy/audio_encoder_resource.h (right): https://codereview.chromium.org/1348563003/diff/240001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.h:119: MediaStreamBufferManager buffer_manager_; On 2015/11/04 01:00:00, bbudge wrote: > s/buffer_manager_/audio_buffer_manager_ for consistency in this class and in the > host. Done.
Patchset #12 (id:260001) has been deleted
Hi Lionel, a few small comments and a larger one. I don't see why the audio buffers are lazily initialized while the bitstream buffers are created at initialization time. That seems backwards, since the client will need audio buffers before bitstream buffers. It seems like it would be better to either create both shared memory handles at initialization time, or lazily create both the audio and bitstream buffers. That would simplify the code since you could do the buffer size calculations together. https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/240001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:313: if (encoder_->Initialize(parameters)) { On 2015/11/04 14:44:27, llandwerlin wrote: > On 2015/11/04 00:59:59, bbudge wrote: > > It's unfortunate that you validate 'parameters' after you've passed them to > the > > encoder. Opus seems to be safe, but chrome internal code isn't generally meant > > to deal with input from potentially malicious code. Could you do all the > > valdiation earlier in this method, using some upper bound on > > encoder_->GetNumberOfSamplesPerFrame()? An large upper bound seems feasible > > since with a size_t, reasonable parameters shouldn't be close to overflowing. > > The parameters are validated above line 304, those is just additional checks. > I put them to be consistent with the input buffers allocation. > > Following your argument, one could say that safe math isn't useful for input > buffers allocation either, because the size of the buffers is based upon > validated parameters in this method and the value returned by > GetNumberOfSamplesPerFrame() which is owned by host. > > > > > I'm imagining you could move all your safe math into this method, and store > the > > buffer and memory size results in member fields. On overflows, just return > > failed rather than crashing the renderer process. This would also allow you to > > eliminate the GetAudioBufferSize method. I think that would be clearer, and > more > > robust than scattering the checks in different methods, which execute at > > different points in the lifetime of the host. > > I just realized I can remove the parameters of the Allocate* methods because all > of them can be inferred from the initialization parameters. OK, I missed that the parameters have to match a supported profile. https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:40: using BitstreamBufferReadyCB = base::Callback<void(int32_t size)>; Chromium idiom is to use typedef. https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:69: uint32_t* buffer_id) { Buffer id's are defined by MediaStreamBufferManager to be int32_t. Could you convert your uint32_t typing to save some casting? https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:89: // occured. Move comment to callback typedef. https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:137: } If your intention is to delay releasing buffer memory until all media tasks have completed, you can do this: media_task_runner_->DeleteSoon(FROM_HERE, encoder_memory_.release());
The reason for the lazily allocated audio buffers is because there was hope to reuse the buffers from the MediaStreamAudioTrack directly at some point (much like what's done in the VideoEncoder). Changed now and dropped the IPC messages too. https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:40: using BitstreamBufferReadyCB = base::Callback<void(int32_t size)>; On 2015/11/10 00:08:25, bbudge wrote: > Chromium idiom is to use typedef. I was told to use "using" earlier : https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:69: uint32_t* buffer_id) { On 2015/11/10 00:08:25, bbudge wrote: > Buffer id's are defined by MediaStreamBufferManager to be int32_t. Could you > convert your uint32_t typing to save some casting? Ok, the reason for this is that IPC messages have uint32_t types. I can't see any case where a negative integer would be transmitted between processes for a buffer id. So I will need a static_cast when sending the message. https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:89: // occured. On 2015/11/10 00:08:25, bbudge wrote: > Move comment to callback typedef. Done. https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:137: } On 2015/11/10 00:08:25, bbudge wrote: > If your intention is to delay releasing buffer memory until all media tasks have > completed, you can do this: > > media_task_runner_->DeleteSoon(FROM_HERE, encoder_memory_.release()); I want to make sure the media task is finished using the memory buffers (audio and bitstream) before destroying the PepperAudioEncoderHost instance. DeleteSoon would leave an opportunity for the media task to access freed memory.
Patchset #12 (id:280001) has been deleted
Patchset #10 (id:220001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:40: using BitstreamBufferReadyCB = base::Callback<void(int32_t size)>; On 2015/11/10 18:00:31, llandwerlin wrote: > On 2015/11/10 00:08:25, bbudge wrote: > > Chromium idiom is to use typedef. > > I was told to use "using" earlier : > > https://codereview.chromium.org/1348563003/diff/20001/content/renderer/pepper... OK, I see that's new for C++ 11. https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:69: uint32_t* buffer_id) { On 2015/11/10 18:00:31, llandwerlin wrote: > On 2015/11/10 00:08:25, bbudge wrote: > > Buffer id's are defined by MediaStreamBufferManager to be int32_t. Could you > > convert your uint32_t typing to save some casting? > > Ok, the reason for this is that IPC messages have uint32_t types. > I can't see any case where a negative integer would be transmitted between > processes for a buffer id. So I will need a static_cast when sending the > message. I would change the message param types too. You should probably check that they are in range though, or does MediaStreamBufferManager do that for you? https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:137: } On 2015/11/10 18:00:31, llandwerlin wrote: > On 2015/11/10 00:08:25, bbudge wrote: > > If your intention is to delay releasing buffer memory until all media tasks > have > > completed, you can do this: > > > > media_task_runner_->DeleteSoon(FROM_HERE, encoder_memory_.release()); > > I want to make sure the media task is finished using the memory buffers (audio > and bitstream) before destroying the PepperAudioEncoderHost instance. > DeleteSoon would leave an opportunity for the media task to access freed memory. I think the meaning of DeleteSoon is that a task is posted to do the delete, so all media tasks should have completed by that point.
https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:69: uint32_t* buffer_id) { On 2015/11/10 18:58:32, bbudge wrote: > On 2015/11/10 18:00:31, llandwerlin wrote: > > On 2015/11/10 00:08:25, bbudge wrote: > > > Buffer id's are defined by MediaStreamBufferManager to be int32_t. Could you > > > convert your uint32_t typing to save some casting? > > > > Ok, the reason for this is that IPC messages have uint32_t types. > > I can't see any case where a negative integer would be transmitted between > > processes for a buffer id. So I will need a static_cast when sending the > > message. > > I would change the message param types too. You should probably check that they > are in range though, or does MediaStreamBufferManager do that for you? It does CHECKs and returns nullptr for invalid buffer id (so likely to trigger a crash). I'll add bound checking. https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:137: } On 2015/11/10 18:58:32, bbudge wrote: > On 2015/11/10 18:00:31, llandwerlin wrote: > > On 2015/11/10 00:08:25, bbudge wrote: > > > If your intention is to delay releasing buffer memory until all media tasks > > have > > > completed, you can do this: > > > > > > media_task_runner_->DeleteSoon(FROM_HERE, encoder_memory_.release()); > > > > I want to make sure the media task is finished using the memory buffers (audio > > and bitstream) before destroying the PepperAudioEncoderHost instance. > > DeleteSoon would leave an opportunity for the media task to access freed > memory. > > I think the meaning of DeleteSoon is that a task is posted to do the delete, so > all media tasks should have completed by that point. Yes, I thought about using it after your comment above but then I remembered that we actually need to wait for the task to complete before we can free the audio & bitstream buffers owned by PepperAudioEncoderHost.
https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/300001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:137: } On 2015/11/11 11:36:49, llandwerlin wrote: > On 2015/11/10 18:58:32, bbudge wrote: > > On 2015/11/10 18:00:31, llandwerlin wrote: > > > On 2015/11/10 00:08:25, bbudge wrote: > > > > If your intention is to delay releasing buffer memory until all media > tasks > > > have > > > > completed, you can do this: > > > > > > > > media_task_runner_->DeleteSoon(FROM_HERE, encoder_memory_.release()); > > > > > > I want to make sure the media task is finished using the memory buffers > (audio > > > and bitstream) before destroying the PepperAudioEncoderHost instance. > > > DeleteSoon would leave an opportunity for the media task to access freed > > memory. > > > > I think the meaning of DeleteSoon is that a task is posted to do the delete, > so > > all media tasks should have completed by that point. > > Yes, I thought about using it after your comment above but then I remembered > that we actually need to wait for the task to complete before we can free the > audio & bitstream buffers owned by PepperAudioEncoderHost. I think this will need to be reworked then. The problem is that this code runs as part of the host destructor, and will block the main renderer thread while the encode tasks complete, which could potentially be a long time. Your basic idea here is good: keep the buffer memory alive. You actually don't need to keep the AudioEncoderImpl alive, since it's referenced on the other thread via weak ptrs. In fact, it's better that you don't keep it alive, to prevent the callbacks from even running. Probably the cleanest thing to do is to post a task from the host destructor to destroy the buffer managers after the encode tasks complete. You could also do that in the AudioEncoderImpl class via a Destroy method, since you have a ref to the media_task_runner_ there already. The host destructor would then run the AudioEncoderImpl destructor, which would prevent any future tasks from running. Any in-progress tasks would still be able to write to the buffers since they're the last thing to be destroyed. https://codereview.chromium.org/1348563003/diff/340001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/340001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:218: FROM_HERE, base::Bind(&AudioEncoderImpl::RequestBitrateChange, Should be DoRequestBitrateChange.
Thanks, reworked the destruction sequence by freeing buffers & encoder on the media thread like you suggested. https://codereview.chromium.org/1348563003/diff/340001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/340001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:218: FROM_HERE, base::Bind(&AudioEncoderImpl::RequestBitrateChange, On 2015/11/11 22:43:18, bbudge wrote: > Should be DoRequestBitrateChange. Ooops, done.
Patchset #10 (id:320001) has been deleted
Patchset #9 (id:300001) has been deleted
https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:115: PepperAudioEncoderHost::AudioEncoderImpl::AudioEncoderImpl() {} Could you initialize opus_encoder_(NULL) and parameters here? https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:137: const ppapi::proxy::PPB_AudioEncodeParameters& parameters) { Could the body be reworked here so that fields are not left in partially initialized state if we fail? In other words, only assign to encoder_memory_, opus_encoder_, and parameters_ once you know Initialize succeeded. Also, in methods, could you DCHECK(opus_encoder_) or some similar proxy for success? Of course GetSupportedProfiles is the exception, since it is called before Initialize. In fact, that method could be made static since it shouldn't need any state. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:166: int32_t PepperAudioEncoderHost::AudioEncoderImpl::GetNumberOfSamplesPerFrame() { DCHECK() that Initialize succeeded for sanity here and other methods. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:196: bitstream_buffer_manager_(new ppapi::MediaStreamBufferManager(this)), It's probably better to defer constructing these until you are actually allocating the buffers. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:249: encode_parameters_ = parameters; nit: s/encode_parameters_/parameters_ for consistency with the AudioEncoderImpl, since there are no other kind of parameters in the host. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:251: int32_t error = PP_ERROR_FAILED; I think the initialization code is clearer if you do the inverse tests, i.e. if (parameters.acceleration != PP_HARDWAREACCELERATION_NONE && parameters.acceleration != PP_HARDWAREACCELERATION_WITHFALLBACK) return PP_ERROR_FAILED; scoped_ptr<AudioEncoderImpl> encoder(new ... if (!encoder_->Initialize(parameters)) return PP_ERROR_FAILED; if (!AllocateBuffers()) return PP_ERROR_NOMEMORY; encode_parameters_ = parameters; encoder_.swap(encoder); ... https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:350: *profiles = software_encoder.GetSupportedProfiles(); Looks like this should be a static method. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:384: AllocateBitstreamBuffers(buffer_size); I think it would be better to inline these function calls. As it is you may leave one buffer initialized and another uninitialized if one call fails. Also, it's strange to pass a base::CheckedNumeric as a function parameter. I can't find any other instances of this in Chromium. Doing everything inline should shorten the calculations and will also make it clearer how the audio and bitstream buffer sizes are related. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:403: ->HostAllocateSharedMemoryBuffer(total_memory_size.ValueOrDie()) This could return a null pointer, and SetBuffers doesn't check for that, so you need a null check here. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:456: &bitstream_buffer_id)); I think this would be clearer if you just inline the functions here. Also, I would consider eliminating the AudioData struct and passing the parameters separately. Is there an issue with parameter count in the binding code?
https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:115: PepperAudioEncoderHost::AudioEncoderImpl::AudioEncoderImpl() {} On 2015/11/12 20:31:31, bbudge wrote: > Could you initialize opus_encoder_(NULL) and parameters here? Done. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:137: const ppapi::proxy::PPB_AudioEncodeParameters& parameters) { On 2015/11/12 20:31:31, bbudge wrote: > Could the body be reworked here so that fields are not left in partially > initialized state if we fail? In other words, only assign to encoder_memory_, > opus_encoder_, and parameters_ once you know Initialize succeeded. > > Also, in methods, could you DCHECK(opus_encoder_) or some similar proxy for > success? Of course GetSupportedProfiles is the exception, since it is called > before Initialize. In fact, that method could be made static since it shouldn't > need any state. Ok, I think the DCHECK should be on encoder_memory_. I'll add a comment in the class to let the reader know that all the variables are only valid if encoder_memory_ is not null. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:166: int32_t PepperAudioEncoderHost::AudioEncoderImpl::GetNumberOfSamplesPerFrame() { On 2015/11/12 20:31:31, bbudge wrote: > DCHECK() that Initialize succeeded for sanity here and other methods. Done. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:196: bitstream_buffer_manager_(new ppapi::MediaStreamBufferManager(this)), On 2015/11/12 20:31:31, bbudge wrote: > It's probably better to defer constructing these until you are actually > allocating the buffers. Done. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:249: encode_parameters_ = parameters; On 2015/11/12 20:31:31, bbudge wrote: > nit: s/encode_parameters_/parameters_ for consistency with the AudioEncoderImpl, > since there are no other kind of parameters in the host. Done. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:251: int32_t error = PP_ERROR_FAILED; On 2015/11/12 20:31:31, bbudge wrote: > I think the initialization code is clearer if you do the inverse tests, i.e. > > if (parameters.acceleration != PP_HARDWAREACCELERATION_NONE && > parameters.acceleration != PP_HARDWAREACCELERATION_WITHFALLBACK) > return PP_ERROR_FAILED; > scoped_ptr<AudioEncoderImpl> encoder(new ... > if (!encoder_->Initialize(parameters)) > return PP_ERROR_FAILED; > if (!AllocateBuffers()) > return PP_ERROR_NOMEMORY; > > encode_parameters_ = parameters; > encoder_.swap(encoder); > ... Thanks, done. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:350: *profiles = software_encoder.GetSupportedProfiles(); On 2015/11/12 20:31:31, bbudge wrote: > Looks like this should be a static method. Done. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:384: AllocateBitstreamBuffers(buffer_size); On 2015/11/12 20:31:31, bbudge wrote: > I think it would be better to inline these function calls. As it is you may > leave one buffer initialized and another uninitialized if one call fails. > > Also, it's strange to pass a base::CheckedNumeric as a function parameter. I > can't find any other instances of this in Chromium. > > Doing everything inline should shorten the calculations and will also make it > clearer how the audio and bitstream buffer sizes are related. Done. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:403: ->HostAllocateSharedMemoryBuffer(total_memory_size.ValueOrDie()) On 2015/11/12 20:31:31, bbudge wrote: > This could return a null pointer, and SetBuffers doesn't check for that, so you > need a null check here. Done. https://codereview.chromium.org/1348563003/diff/360001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:456: &bitstream_buffer_id)); On 2015/11/12 20:31:31, bbudge wrote: > I think this would be clearer if you just inline the functions here. > > Also, I would consider eliminating the AudioData struct and passing the > parameters separately. Is there an issue with parameter count in the binding > code? Done.
https://codereview.chromium.org/1348563003/diff/380001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/380001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:55: // occured. nit: spelling occurred https://codereview.chromium.org/1348563003/diff/380001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:390: // buffers are twice the size of the raw data). Why are the bitstream buffers twice the size? I'm assuming that it's to handle pathological inputs where compression doesn't work. It would be good to explain that here, if you haven't somewhere else. https://codereview.chromium.org/1348563003/diff/380001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:402: scoped_ptr<base::SharedMemory> bistream_memory( nit: bitstream https://codereview.chromium.org/1348563003/diff/380001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.h (right): https://codereview.chromium.org/1348563003/diff/380001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:29: class AudioEncoderImpl; should be first private decl. https://codereview.chromium.org/1348563003/diff/380001/ppapi/proxy/audio_enco... File ppapi/proxy/audio_encoder_resource.cc (right): https://codereview.chromium.org/1348563003/diff/380001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:21: if (!TrackedCallback::IsPending(*callback)) In this case I think it's clearer as: if (TrackedCallback::IsPending... { } rather than early return. https://codereview.chromium.org/1348563003/diff/380001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:31: class AudioEncoderResource::BitstreamBufferManager { It would be great if you could use MediaStreamBufferManager for the bitstream buffers. It looks like you've added the ability to associate an id (audio buffer) with each bitstream buffer. I think you could achieve this outside the buffer manager, with a map in the host. https://codereview.chromium.org/1348563003/diff/380001/ppapi/proxy/audio_enco... File ppapi/proxy/audio_encoder_resource.h (right): https://codereview.chromium.org/1348563003/diff/380001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.h:102: typedef std::map<PP_Resource, scoped_refptr<AudioBufferResource>> Should these be 'using' as well?
https://codereview.chromium.org/1348563003/diff/380001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/380001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:55: // occured. On 2015/11/13 20:37:56, bbudge wrote: > nit: spelling occurred Done. https://codereview.chromium.org/1348563003/diff/380001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:390: // buffers are twice the size of the raw data). On 2015/11/13 20:37:56, bbudge wrote: > Why are the bitstream buffers twice the size? I'm assuming that it's to handle > pathological inputs where compression doesn't work. It would be good to explain > that here, if you haven't somewhere else. Yes, that's exactly why. Adding a comment here. https://codereview.chromium.org/1348563003/diff/380001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:402: scoped_ptr<base::SharedMemory> bistream_memory( On 2015/11/13 20:37:56, bbudge wrote: > nit: bitstream Done. https://codereview.chromium.org/1348563003/diff/380001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.h (right): https://codereview.chromium.org/1348563003/diff/380001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:29: class AudioEncoderImpl; On 2015/11/13 20:37:56, bbudge wrote: > should be first private decl. Done. https://codereview.chromium.org/1348563003/diff/380001/ppapi/proxy/audio_enco... File ppapi/proxy/audio_encoder_resource.cc (right): https://codereview.chromium.org/1348563003/diff/380001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:21: if (!TrackedCallback::IsPending(*callback)) On 2015/11/13 20:37:56, bbudge wrote: > In this case I think it's clearer as: if (TrackedCallback::IsPending... { } > rather than early return. Done. https://codereview.chromium.org/1348563003/diff/380001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:31: class AudioEncoderResource::BitstreamBufferManager { On 2015/11/13 20:37:56, bbudge wrote: > It would be great if you could use MediaStreamBufferManager for the bitstream > buffers. It looks like you've added the ability to associate an id (audio > buffer) with each bitstream buffer. I think you could achieve this outside the > buffer manager, with a map in the host. Done. https://codereview.chromium.org/1348563003/diff/380001/ppapi/proxy/audio_enco... File ppapi/proxy/audio_encoder_resource.h (right): https://codereview.chromium.org/1348563003/diff/380001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.h:102: typedef std::map<PP_Resource, scoped_refptr<AudioBufferResource>> On 2015/11/13 20:37:56, bbudge wrote: > Should these be 'using' as well? Done.
+ a general question: Did you consider defining a new kind of MediaStreamBuffer type for Bitstream buffers? Then the size could be part of its header. It would waste a few bytes of buffer space, but you could simplify your messages and eliminate your queue of buffer sizes in the resource. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:220: parameters_ = parameters; I don't think this field is actually used in the host class. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:330: profile.hardware_accelerated == PP_TRUE ? true : false, nit: this is equivalent to just profile.hardware_accelerated == PP_TRUE https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:340: int32_t samples_per_frame) { Suggestion for the body of this method: Do all size computations first to make sure everything's OK before allocating the shared memory and creating the buffer managers. Also, you only need to check IsValid on the largest total buffer sizes. You can get rid of 2 IsValid checks that way. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:354: total_audio_buffer_size.ValueOrDie(); No need for ValueOrDie here. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:385: // buffers are twice the size of the raw data, to handle the worse case nit: worst https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:419: DCHECK(encoder_); It seems strange that this is the only place you DCHECK encoder_. Perhaps DCHECK(!encoder_last_error_)? https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:463: bitstream_buffer_id, static_cast<uint32_t>(size))); size is int32_t everywhere else. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:491: scoped_ptr<ppapi::MediaStreamBufferManager> bitstream_buffer_manager) {} Make free function. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.h (right): https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:61: base::CheckedNumeric<size_t> GetAudioBufferSize(); Delete this. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:75: scoped_ptr<ppapi::MediaStreamBufferManager> bitstream_buffer_manager); Sorry I didn't think of this earlier, but couldn't this be a free function defined in the anonymous namespace of the .cc file? Then you can remove this. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:101: // Media task used to run the encoder. s/task/task runner https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... File ppapi/proxy/audio_encoder_resource.cc (right): https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:35: closed_(false), initialize number_of_samples_ and get_buffer_data_ to some known value. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:35: closed_(false), closed_ is written but never read, so you can remove it. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:133: buffer_resource->Invalidate(); The real reason to call Invalidate() is so the resource won't CHECK when it's destructed. It checks for leaked buffers. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:205: NotifyError(error); It seems wrong to NotifyError in this case. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:211: RunCallback(&get_supported_profiles_callback_, PP_ERROR_BADARGUMENT); In this case, we usually return PP_ERROR_FAILED since the argument was OK but it didn't give us memory. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:220: } You could eliminate some duplication like this: ArrayWriter writer(output); if (params.result() != PP_OK || !writer.is_valid() || !writer.StoreVector(profiles)) { RunCallback(&get_supported_profiles_callback_, PP_ERROR_FAILED); return; } https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:237: NotifyError(error); I guess this works but you could also just call: RunCallback(&initialize_callback_, error); since there are no pending callbacks at this point, and encoder_last_error_ is PP_ERROR_FAILED. here and below. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:244: NotifyError(PP_ERROR_FAILED); PP_ERROR_NOMEMORY here and below https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:256: // Get bitstreamer buffers shared memory buffer. s/bitstreamer/bitstream https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:278: RunCallback(&initialize_callback_, encoder_last_error_); RunCallback(&initialize_callback_, PP_OK); https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:280: if (TrackedCallback::IsPending(get_buffer_callback_)) This should never be true so you could remove it. The plugin can't call GetBuffer until we set encoder_last_error_ in 274. If the plugin calls GetBuffer in its Initialize callback, it will return the first buffer immediately. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:293: encoder_last_error_ = params.result(); Looking at the host, this is an unsolicited reply so the result should be PP_OK. Or am I missing something? https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:326: void AudioEncoderResource::NotifyError(int32_t error) { DCHECK(error) might be a good idea here. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:337: RunCallback(&callback, error); RunCallback(&it->second, error); // doesn't seem less readable to me. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:342: void AudioEncoderResource::TryWriteAudioBuffer() { I'm finding this method name confusing. How about TryGetBuffer or TryGetAudioBuffer? https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:355: *get_buffer_data_ = resource->GetReference(); This line deserves a comment: // Take a reference for the plugin. or something to that effect. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:371: static_cast<void*>(bitstream_buffer_manager_.GetBufferPointer(buffer_id)); Is this cast needed? https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:381: it->second = nullptr; This line isn't strictly needed. clear() will cause all refptr's to be destructed properly. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... File ppapi/proxy/audio_encoder_resource.h (right): https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.h:103: AudioBufferMap; nit: using https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.h:117: // Buffer manager for audio frames. I wouldn't complain if you dropped the comments on the buffer managers :) https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.h:126: // Map of bitstream buffer point to buffer ids. s/point/pointers
Here is a version with the add bitstream buffer type in MediaStreamBuffer. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:220: parameters_ = parameters; On 2015/11/17 00:53:40, bbudge wrote: > I don't think this field is actually used in the host class. Done. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:330: profile.hardware_accelerated == PP_TRUE ? true : false, On 2015/11/17 00:53:39, bbudge wrote: > nit: this is equivalent to just profile.hardware_accelerated == PP_TRUE Done. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:340: int32_t samples_per_frame) { On 2015/11/17 00:53:39, bbudge wrote: > Suggestion for the body of this method: Do all size computations first to make > sure everything's OK before allocating the shared memory and creating the buffer > managers. Also, you only need to check IsValid on the largest total buffer > sizes. You can get rid of 2 IsValid checks that way. Done. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:354: total_audio_buffer_size.ValueOrDie(); On 2015/11/17 00:53:40, bbudge wrote: > No need for ValueOrDie here. Done. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:385: // buffers are twice the size of the raw data, to handle the worse case On 2015/11/17 00:53:39, bbudge wrote: > nit: worst Done. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:419: DCHECK(encoder_); On 2015/11/17 00:53:40, bbudge wrote: > It seems strange that this is the only place you DCHECK encoder_. Perhaps > DCHECK(!encoder_last_error_)? Done. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:463: bitstream_buffer_id, static_cast<uint32_t>(size))); On 2015/11/17 00:53:40, bbudge wrote: > size is int32_t everywhere else. Done. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:491: scoped_ptr<ppapi::MediaStreamBufferManager> bitstream_buffer_manager) {} On 2015/11/17 00:53:40, bbudge wrote: > Make free function. Are you asking to put the function in an anonymous namespace? It was like that before but I had to change it when AudioEncoderImpl became private. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.h (right): https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:61: base::CheckedNumeric<size_t> GetAudioBufferSize(); On 2015/11/17 00:53:40, bbudge wrote: > Delete this. Sorry, done. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:75: scoped_ptr<ppapi::MediaStreamBufferManager> bitstream_buffer_manager); On 2015/11/17 00:53:40, bbudge wrote: > Sorry I didn't think of this earlier, but couldn't this be a free function > defined in the anonymous namespace of the .cc file? Then you can remove this. The problem is that AudioEncoderImpl is private to PepperAudioEncoderHost, so a free function won't compile with this prototype. Maybe there is trick for that? https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.h:101: // Media task used to run the encoder. On 2015/11/17 00:53:40, bbudge wrote: > s/task/task runner Done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... File ppapi/proxy/audio_encoder_resource.cc (right): https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:133: buffer_resource->Invalidate(); On 2015/11/17 00:53:40, bbudge wrote: > The real reason to call Invalidate() is so the resource won't CHECK when it's > destructed. It checks for leaked buffers. Done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:205: NotifyError(error); On 2015/11/17 00:53:40, bbudge wrote: > It seems wrong to NotifyError in this case. It seems that if the host is not able to answer the supported profiles, something is going wrong that will make any subsequent interaction fail, but ok. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:211: RunCallback(&get_supported_profiles_callback_, PP_ERROR_BADARGUMENT); On 2015/11/17 00:53:40, bbudge wrote: > In this case, we usually return PP_ERROR_FAILED since the argument was OK but it > didn't give us memory. Done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:220: } On 2015/11/17 00:53:40, bbudge wrote: > You could eliminate some duplication like this: > > ArrayWriter writer(output); > if (params.result() != PP_OK || !writer.is_valid() || > !writer.StoreVector(profiles)) { > RunCallback(&get_supported_profiles_callback_, PP_ERROR_FAILED); > return; > } > Done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:237: NotifyError(error); On 2015/11/17 00:53:41, bbudge wrote: > I guess this works but you could also just call: > RunCallback(&initialize_callback_, error); > > since there are no pending callbacks at this point, and encoder_last_error_ is > PP_ERROR_FAILED. > > here and below. Done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:244: NotifyError(PP_ERROR_FAILED); On 2015/11/17 00:53:40, bbudge wrote: > PP_ERROR_NOMEMORY > here and below Done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:256: // Get bitstreamer buffers shared memory buffer. On 2015/11/17 00:53:40, bbudge wrote: > s/bitstreamer/bitstream Done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:278: RunCallback(&initialize_callback_, encoder_last_error_); On 2015/11/17 00:53:40, bbudge wrote: > RunCallback(&initialize_callback_, PP_OK); Done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:280: if (TrackedCallback::IsPending(get_buffer_callback_)) On 2015/11/17 00:53:40, bbudge wrote: > This should never be true so you could remove it. > > The plugin can't call GetBuffer until we set encoder_last_error_ in 274. If the > plugin calls GetBuffer in its Initialize callback, it will return the first > buffer immediately. Thanks, done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:293: encoder_last_error_ = params.result(); On 2015/11/17 00:53:40, bbudge wrote: > Looking at the host, this is an unsolicited reply so the result should be PP_OK. > Or am I missing something? Thanks, done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:326: void AudioEncoderResource::NotifyError(int32_t error) { On 2015/11/17 00:53:40, bbudge wrote: > DCHECK(error) > > might be a good idea here. Done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:337: RunCallback(&callback, error); On 2015/11/17 00:53:41, bbudge wrote: > RunCallback(&it->second, error); // doesn't seem less readable to me. Done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:342: void AudioEncoderResource::TryWriteAudioBuffer() { On 2015/11/17 00:53:40, bbudge wrote: > I'm finding this method name confusing. How about TryGetBuffer or > TryGetAudioBuffer? Done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:355: *get_buffer_data_ = resource->GetReference(); On 2015/11/17 00:53:40, bbudge wrote: > This line deserves a comment: > // Take a reference for the plugin. > > or something to that effect. Done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:381: it->second = nullptr; On 2015/11/17 00:53:40, bbudge wrote: > This line isn't strictly needed. clear() will cause all refptr's to be > destructed properly. Done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... File ppapi/proxy/audio_encoder_resource.h (right): https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.h:103: AudioBufferMap; On 2015/11/17 00:53:41, bbudge wrote: > nit: using Done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.h:117: // Buffer manager for audio frames. On 2015/11/17 00:53:41, bbudge wrote: > I wouldn't complain if you dropped the comments on the buffer managers :) Done. https://codereview.chromium.org/1348563003/diff/400001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.h:126: // Map of bitstream buffer point to buffer ids. On 2015/11/17 00:53:41, bbudge wrote: > s/point/pointers Done.
Starting to look good. https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/400001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:491: scoped_ptr<ppapi::MediaStreamBufferManager> bitstream_buffer_manager) {} On 2015/11/17 15:22:30, llandwerlin wrote: > On 2015/11/17 00:53:40, bbudge wrote: > > Make free function. > > Are you asking to put the function in an anonymous namespace? > It was like that before but I had to change it when AudioEncoderImpl became > private. I see now. It's fine like this. https://codereview.chromium.org/1348563003/diff/420001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/420001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:220: if (parameters.acceleration == PP_HARDWAREACCELERATION_ONLY) This should be caught in IsInitializationValid. https://codereview.chromium.org/1348563003/diff/420001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:261: return PP_ERROR_BADARGUMENT; The resource checks for a bad buffer resource. This code is to catch malicious plugihs so just a curt PP_ERROR_FAILED is better here. And below. https://codereview.chromium.org/1348563003/diff/420001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:434: audio_buffer->audio.data_size, A malicious plugin could modify the header we initialized in the audio buffer in order to confuse the encoder, so we shouldn't trust that the header is valid here. Instead, store the buffer sizes as fields on the host and use those here. https://codereview.chromium.org/1348563003/diff/420001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:436: bitstream_buffer->header.size, Ditto. https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... File ppapi/proxy/audio_encoder_resource.cc (right): https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:35: closed_(false), Remove. https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:178: closed_ = true; Remove. https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:241: // Get bitstreamer buffers shared memory buffer. s/bitstreamer/bitstream https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:266: // processing this message. We might receive a EncodeReply message tiny nit: s/a/an https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... File ppapi/proxy/audio_encoder_resource.h (right): https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.h:37: class BitstreamBufferManager; Remove. https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.h:97: bool closed_; This isn't necessary.
https://codereview.chromium.org/1348563003/diff/420001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/420001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:261: return PP_ERROR_BADARGUMENT; On 2015/11/17 20:23:59, bbudge wrote: > The resource checks for a bad buffer resource. This code is to catch malicious > plugihs so just a curt PP_ERROR_FAILED is better here. And below. Done. https://codereview.chromium.org/1348563003/diff/420001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:434: audio_buffer->audio.data_size, On 2015/11/17 20:23:59, bbudge wrote: > A malicious plugin could modify the header we initialized in the audio buffer in > order to confuse the encoder, so we shouldn't trust that the header is valid > here. Instead, store the buffer sizes as fields on the host and use those here. Thanks, I just realized the size isn't right either... https://codereview.chromium.org/1348563003/diff/420001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:436: bitstream_buffer->header.size, On 2015/11/17 20:23:59, bbudge wrote: > Ditto. Done. https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... File ppapi/proxy/audio_encoder_resource.cc (right): https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:35: closed_(false), On 2015/11/17 20:23:59, bbudge wrote: > Remove. Done. https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:178: closed_ = true; On 2015/11/17 20:23:59, bbudge wrote: > Remove. Done. https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:241: // Get bitstreamer buffers shared memory buffer. On 2015/11/17 20:23:59, bbudge wrote: > s/bitstreamer/bitstream Done. https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.cc:266: // processing this message. We might receive a EncodeReply message On 2015/11/17 20:23:59, bbudge wrote: > tiny nit: s/a/an Done. https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... File ppapi/proxy/audio_encoder_resource.h (right): https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.h:37: class BitstreamBufferManager; On 2015/11/17 20:23:59, bbudge wrote: > Remove. Done. https://codereview.chromium.org/1348563003/diff/420001/ppapi/proxy/audio_enco... ppapi/proxy/audio_encoder_resource.h:97: bool closed_; On 2015/11/17 20:23:59, bbudge wrote: > This isn't necessary. Done.
A few more comments, otherwise LGTM. https://codereview.chromium.org/1348563003/diff/440001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/440001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:220: if (parameters.acceleration == PP_HARDWAREACCELERATION_ONLY) Never true given the current AudioEncoderImpl. https://codereview.chromium.org/1348563003/diff/440001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:447: int32_t size) { nit: Maybe 'result' is a better name, to be consistent with AudioEncoderImpl::Encode and the fact that it may be an error code. https://codereview.chromium.org/1348563003/diff/440001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:456: if (size < 0) { Perhaps this should be before the EncodeReply. Otherwise, it looks like the encode succeeded to the plugin. https://codereview.chromium.org/1348563003/diff/440001/ppapi/tests/test_audio... File ppapi/tests/test_audio_encoder.cc (right): https://codereview.chromium.org/1348563003/diff/440001/ppapi/tests/test_audio... ppapi/tests/test_audio_encoder.cc:24: RUN_CALLBACK_TEST(TestAudioEncoder, InitializeOpusMono, filter); test TestInitializeOpusStereo https://codereview.chromium.org/1348563003/diff/440001/ppapi/tests/test_audio... ppapi/tests/test_audio_encoder.cc:64: 42, static_cast<PP_AudioBuffer_SampleRate>(48000), PP_AUDIOBUFFER_SAMPLERATE_48000 https://codereview.chromium.org/1348563003/diff/440001/ppapi/tests/test_audio... ppapi/tests/test_audio_encoder.cc:72: static_cast<PP_AudioBuffer_SampleSize>(2), PP_AUDIOPROFILE_OPUS, 10000, PP_AUDIOBUFFER_SAMPLESIZE_16_BITS
https://codereview.chromium.org/1348563003/diff/440001/content/renderer/peppe... File content/renderer/pepper/pepper_audio_encoder_host.cc (right): https://codereview.chromium.org/1348563003/diff/440001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:220: if (parameters.acceleration == PP_HARDWAREACCELERATION_ONLY) On 2015/11/18 17:57:57, bbudge wrote: > Never true given the current AudioEncoderImpl. Done. https://codereview.chromium.org/1348563003/diff/440001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:447: int32_t size) { On 2015/11/18 17:57:57, bbudge wrote: > nit: Maybe 'result' is a better name, to be consistent with > AudioEncoderImpl::Encode and the fact that it may be an error code. Done. https://codereview.chromium.org/1348563003/diff/440001/content/renderer/peppe... content/renderer/pepper/pepper_audio_encoder_host.cc:456: if (size < 0) { On 2015/11/18 17:57:57, bbudge wrote: > Perhaps this should be before the EncodeReply. Otherwise, it looks like the > encode succeeded to the plugin. Done. https://codereview.chromium.org/1348563003/diff/440001/ppapi/tests/test_audio... File ppapi/tests/test_audio_encoder.cc (right): https://codereview.chromium.org/1348563003/diff/440001/ppapi/tests/test_audio... ppapi/tests/test_audio_encoder.cc:24: RUN_CALLBACK_TEST(TestAudioEncoder, InitializeOpusMono, filter); On 2015/11/18 17:57:57, bbudge wrote: > test TestInitializeOpusStereo Done. https://codereview.chromium.org/1348563003/diff/440001/ppapi/tests/test_audio... ppapi/tests/test_audio_encoder.cc:64: 42, static_cast<PP_AudioBuffer_SampleRate>(48000), On 2015/11/18 17:57:57, bbudge wrote: > PP_AUDIOBUFFER_SAMPLERATE_48000 Done. https://codereview.chromium.org/1348563003/diff/440001/ppapi/tests/test_audio... ppapi/tests/test_audio_encoder.cc:72: static_cast<PP_AudioBuffer_SampleSize>(2), PP_AUDIOPROFILE_OPUS, 10000, On 2015/11/18 17:57:57, bbudge wrote: > PP_AUDIOBUFFER_SAMPLESIZE_16_BITS Done.
Patchset #15 (id:460001) has been deleted
The CQ bit was checked by lionel.g.landwerlin@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, kinuko@chromium.org, bbudge@chromium.org Link to the patchset: https://codereview.chromium.org/1348563003/#ps480001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1348563003/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1348563003/480001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== ppapi: implement PPB_AudioEncoder BUG=461222 ========== to ========== ppapi: implement PPB_AudioEncoder BUG=461222 ==========
lionel.g.landwerlin@intel.com changed reviewers: + sergeyu@chromium.org
sergeyu@: Could you LGTM this change if you're okay with it. It seems it needs an owner from third_party/opus. Thanks!
lgtm
The CQ bit was checked by lionel.g.landwerlin@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1348563003/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1348563003/480001
Message was sent while issue was closed.
Committed patchset #15 (id:480001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/764a8c9512e79b62621e997aa802a6bcfd2c171c Cr-Commit-Position: refs/heads/master@{#360469}
Message was sent while issue was closed.
akuegel@chromium.org changed reviewers: + akuegel@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1348563003/diff/480001/ppapi/tests/test_audio... File ppapi/tests/test_audio_encoder.cc (right): https://codereview.chromium.org/1348563003/diff/480001/ppapi/tests/test_audio... ppapi/tests/test_audio_encoder.cc:34: TestCompletionCallbackWithOutput<std::vector<PP_AudioProfileDescription>> The two closing '>' break the GN build. Please fix.
Message was sent while issue was closed.
On 2015/11/19 15:54:30, Adrian Kuegel wrote: > https://codereview.chromium.org/1348563003/diff/480001/ppapi/tests/test_audio... > File ppapi/tests/test_audio_encoder.cc (right): > > https://codereview.chromium.org/1348563003/diff/480001/ppapi/tests/test_audio... > ppapi/tests/test_audio_encoder.cc:34: > TestCompletionCallbackWithOutput<std::vector<PP_AudioProfileDescription>> > The two closing '>' break the GN build. Please fix. Actually, I decided to fix it myself: https://codereview.chromium.org/1464513002/ |
