Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
371273004:
Ensures that input Pepper Flash supports the newly added AudioBus interface (Closed)
|
Created:
6 years, 5 months ago by henrika (OOO until Aug 14) Modified:
6 years, 5 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionEnsures that input Pepper Flash supports the newly added AudioBus interface.
This CL (https://codereview.chromium.org/344583002) has modified the data structure in shared memory and the original CL did not take the input side of Pepper Flash into account. Pepper Flash recording was therefore broken. This CL ensures that Pepper knows about the new audio structure and accounts for it using by mapping shared memory to an audio bus and then interleaves the data into a plain byte vector before sending it to the Pepper callback.
Adding vrk@ as TBR for the added dependency in media.
R=piman@chromium.org, xians@chromium.org
TBR=vrk
BUG=387303
TEST=www.youtube.com/my_webcam
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282357
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 22
Patch Set 3 : Changes based on feedback from piman@, tommi@ and xians@ #Patch Set 4 : Now uses static_cast instead of reinterpret_cast #Patch Set 5 : rebased #
Messages
Total messages: 23 (0 generated)
xians@: could you take a look, please.
piman@: I think I need your OK as owner. vrk@: I might need you OK as well since I am adding a dependency to media/base. Note that media/audio was added before. Thanks both!
Henrik, great work to track down the problem :) https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... File ppapi/proxy/audio_input_resource.cc (left): https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:233: while (sizeof(pending_data) == socket_->Receive(&pending_data, may I ask why you changed the condition? It looks the same functionality, or do I miss something? https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... File ppapi/proxy/audio_input_resource.cc (right): https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:188: if (!shared_memory_->Map(shared_memory_size_)) { The code becomes a bit hard to read with the new change, can you change it to: if (shared_memory_->Map(shared_memory_size_)) { // Create a new audio bus and wrap the audio data section in shared memory. Do the setup. } else { PpapiGlobals::Get()->LogWithSource( } https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:249: if you move the audio_wrapper_ setup code here, will you save the sample_frame_count_ and client_buffer_size_bytes_? Like: if (!audio_wrapper_) { audio_wrapper_ = media::AudioBus::WrapMemory( kAudioInputChannels, data_buffer_size, buffer->audio); DCHECK_EQ(buffer->params.size, data_buffer_size * kBitsPerAudioInputSample / 8); client_buffer_.reset(new uint8_t[buffer->params.size]); } https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:274: client_buffer_size_bytes_, client_buffer_size_bytes_ should be the same as buffer->params.size, if so, please previous code instead of making a new member. https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... File ppapi/proxy/audio_input_resource.h (right): https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.h:141: scoped_ptr<media::AudioBus> audio_bus_; change the name to audio_wrapper_, and update the comment.
https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... File ppapi/proxy/audio_input_resource.cc (right): https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:197: reinterpret_cast<media::AudioInputBuffer*>(shared_memory_->memory()); nit: static_cast instead of reinterpret_cast. https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:197: reinterpret_cast<media::AudioInputBuffer*>(shared_memory_->memory()); I see you're sharing metadata (AudioInputBufferParameters) via shared memory between trusted and untrusted side. Can anything bad happen if the untrusted side writes to the shared memory, modifying, e.g. the size? https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:206: kBitsPerAudioInputSample / 8; nit: indent (git cl format?)
drive-by comment https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... File ppapi/proxy/audio_input_resource.cc (right): https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:197: reinterpret_cast<media::AudioInputBuffer*>(shared_memory_->memory()); On 2014/07/08 17:48:35, piman (slow to review) wrote: > nit: static_cast instead of reinterpret_cast. Henrik was asking me about this, so I figured I'd simply reply in the code review, hope you don't mind. From http://en.cppreference.com/w/cpp/language/reinterpret_cast: "Unlike static_cast, but like const_cast, the reinterpret_cast expression does not compile to any CPU instructions. It is purely a compiler directive which instructs the compiler to treat the sequence of bits (object representation) of expression as if it had the type new_type." In this case, memory() returns void* which is not a related type to AudioInputBuffer* so there's nothing the compiler can or should do to convert between these two types, so reinterpret_cast is the right type of cast. static_cast<> won't do anything in practice in exactly this case, but technically, reinterpret_cast<> is correct and in order to not propagate incorrect use of static_cast, I'd like to keep it as is.
Thanks. Adding some comments first. No real changes yet. https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... File ppapi/proxy/audio_input_resource.cc (right): https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:188: if (!shared_memory_->Map(shared_memory_size_)) { On 2014/07/08 15:33:48, xians1 wrote: > The code becomes a bit hard to read with the new change, can you change it to: > if (shared_memory_->Map(shared_memory_size_)) { > // Create a new audio bus and wrap the audio data section in shared memory. > Do the setup. > > } else { > PpapiGlobals::Get()->LogWithSource( > } > Done. https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:197: reinterpret_cast<media::AudioInputBuffer*>(shared_memory_->memory()); Nop. I was following a pattern done in a corresponding output class (see PPB_Audio_Shared). https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:197: reinterpret_cast<media::AudioInputBuffer*>(shared_memory_->memory()); Great comment; please allow me to come back and answer this one next round. Please note that I have not added any meta data in this CL; I just use already existing as before. I'll try to improve by adding some DCHECKs here. https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:206: kBitsPerAudioInputSample / 8; I did run git cl format and it does suggest this indentation. Do you want me to change it anyhow? https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:249: Thanks for the suggestion but, as stated before, I would like to match the corresponding output code as much as possible. Also, I don't want to create the members on this special audio thread but on the main thread where all other members are created. https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:274: client_buffer_size_bytes_, Actually that is not the case since params.size reflects the size of the audio bus which is in float. A typical case for Pepper is the params.size is 8192 and client_buffer_size_bytes_ is 4096 (since we use 16-bit integers instead of float). https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... File ppapi/proxy/audio_input_resource.h (right): https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.h:141: scoped_ptr<media::AudioBus> audio_bus_; I would like to keep this notation to match the corresponding output code (see ppapi/shared_impl/ppb_audio_shared.cc). They both map shared memory into an audio bus and I have used it as reference here. Makes it easier to compare and harmonize in the future. Hope that is OK.
https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... File ppapi/proxy/audio_input_resource.cc (right): https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:188: if (!shared_memory_->Map(shared_memory_size_)) { I followed the example on the output side. I will try to make it more clear.
https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... File ppapi/proxy/audio_input_resource.cc (right): https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:197: reinterpret_cast<media::AudioInputBuffer*>(shared_memory_->memory()); Thanks Tommi; will stick to reinterpret unless piman@ has other input.
piman@ and xians@, PTAL. I have tried to account for all valuable feedback in PS3. piman@: I have not modified how meta data is used but I have tried to add some extra checks to guarantee that nothing bad happens if something would change on the fly. We have used an identical scheme for WebRTC a long time and it seems stable. Also, similar meta data is already used on the output side for Pepper. I am unable to come up with a new, even more secure, scheme in this CL since I want to keep the change as small as possible.
lgtm
On Wed, Jul 9, 2014 at 2:32 AM, <tommi@chromium.org> wrote: > drive-by comment > > > > https://codereview.chromium.org/371273004/diff/40001/ > ppapi/proxy/audio_input_resource.cc > File ppapi/proxy/audio_input_resource.cc (right): > > https://codereview.chromium.org/371273004/diff/40001/ > ppapi/proxy/audio_input_resource.cc#newcode197 > ppapi/proxy/audio_input_resource.cc:197: > reinterpret_cast<media::AudioInputBuffer*>(shared_memory_->memory()); > On 2014/07/08 17:48:35, piman (slow to review) wrote: > >> nit: static_cast instead of reinterpret_cast. >> > > Henrik was asking me about this, so I figured I'd simply reply in the > code review, hope you don't mind. > > From http://en.cppreference.com/w/cpp/language/reinterpret_cast: > "Unlike static_cast, but like const_cast, the reinterpret_cast > expression does not compile to any CPU instructions. It is purely a > compiler directive which instructs the compiler to treat the sequence of > bits (object representation) of expression as if it had the type > new_type." > > In this case, memory() returns void* which is not a related type to > AudioInputBuffer* so there's nothing the compiler can or should do to > convert between these two types, so reinterpret_cast is the right type > of cast. static_cast<> won't do anything in practice in exactly this > case, but technically, reinterpret_cast<> is correct and in order to not > propagate incorrect use of static_cast, I'd like to keep it as is. > We've been consistently using static_cast to downcast from void*. They are related in the sense that AudioInputBuffer* implicitly upcasts to void* (think as void being the base class of AudioInputBuffer). In particular, you rely on that on the other side of this - i.e. you're not using reinterpret_cast to cast to void*. If they were not related, the compiler would refuse the static_cast. Actually static_cast is more correct. Nothing ensures that when AudioInputBuffer* is upcasted to void* it keeps the same value (practically it probably is on today's compilers when upcasting to void*, but it's certainly not true when upcasting to a base class when you have multiple inheritance). If it does not, then the later reinterpret_cast would yield a different pointer than the original. > > https://codereview.chromium.org/371273004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Alright, I'll back down. In any case either cast would do the same thing here. On Jul 9, 2014 10:22 PM, "Antoine Labour" <piman@chromium.org> wrote: > > > > On Wed, Jul 9, 2014 at 2:32 AM, <tommi@chromium.org> wrote: > >> drive-by comment >> >> >> >> https://codereview.chromium.org/371273004/diff/40001/ >> ppapi/proxy/audio_input_resource.cc >> File ppapi/proxy/audio_input_resource.cc (right): >> >> https://codereview.chromium.org/371273004/diff/40001/ >> ppapi/proxy/audio_input_resource.cc#newcode197 >> ppapi/proxy/audio_input_resource.cc:197: >> reinterpret_cast<media::AudioInputBuffer*>(shared_memory_->memory()); >> On 2014/07/08 17:48:35, piman (slow to review) wrote: >> >>> nit: static_cast instead of reinterpret_cast. >>> >> >> Henrik was asking me about this, so I figured I'd simply reply in the >> code review, hope you don't mind. >> >> From http://en.cppreference.com/w/cpp/language/reinterpret_cast: >> "Unlike static_cast, but like const_cast, the reinterpret_cast >> expression does not compile to any CPU instructions. It is purely a >> compiler directive which instructs the compiler to treat the sequence of >> bits (object representation) of expression as if it had the type >> new_type." >> >> In this case, memory() returns void* which is not a related type to >> AudioInputBuffer* so there's nothing the compiler can or should do to >> convert between these two types, so reinterpret_cast is the right type >> of cast. static_cast<> won't do anything in practice in exactly this >> case, but technically, reinterpret_cast<> is correct and in order to not >> propagate incorrect use of static_cast, I'd like to keep it as is. >> > > We've been consistently using static_cast to downcast from void*. > They are related in the sense that AudioInputBuffer* implicitly upcasts to > void* (think as void being the base class of AudioInputBuffer). In > particular, you rely on that on the other side of this - i.e. you're not > using reinterpret_cast to cast to void*. If they were not related, the > compiler would refuse the static_cast. > > Actually static_cast is more correct. Nothing ensures that when > AudioInputBuffer* is upcasted to void* it keeps the same value (practically > it probably is on today's compilers when upcasting to void*, but it's > certainly not true when upcasting to a base class when you have multiple > inheritance). If it does not, then the later reinterpret_cast would yield a > different pointer than the original. > > > > >> >> https://codereview.chromium.org/371273004/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... File ppapi/proxy/audio_input_resource.cc (right): https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:197: reinterpret_cast<media::AudioInputBuffer*>(shared_memory_->memory()); On 2014/07/09 09:39:51, henrika (OOO June 23-July 7) wrote: > Great comment; please allow me to come back and answer this one next round. > Please note that I have not added any meta data in this CL; I just use already > existing as before. Possibly, but you're exposing it in new ways (and I want to check that we didn't miss anything before). > I'll try to improve by adding some DCHECKs here. It's not so much about DCHECKs, it's more about having meta-data being writable by the untrusted process. If the trusted process relies on that data not changing / having particular values, then there is an attack surface (that DCHECKs, especially on the untrusted side, won't protect you against) - e.g. if the trusted side recycles the buffer and relies on the size value to be accurate. So I guess the question is: is it the case that the meta-data is write-only by the trusted side (never reads it)? If so we're fine. If the trusted side reads it, does it need to defend against it changing? https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:206: kBitsPerAudioInputSample / 8; On 2014/07/09 09:39:51, henrika (OOO June 23-July 7) wrote: > I did run git cl format and it does suggest this indentation. Do you want me to > change it anyhow? It looks like a bug in git cl format.... line continuations should be +4.
Thanks for the detailed explanation. I have changed from reinterpret to static now.
https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... File ppapi/proxy/audio_input_resource.cc (right): https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:197: reinterpret_cast<media::AudioInputBuffer*>(shared_memory_->memory()); We are fine since the trusted process only writes (never reads) this meta data and the meta data is set up once (during initialization) and is not (or can't be) changed during a session. https://codereview.chromium.org/371273004/diff/40001/ppapi/proxy/audio_input_... ppapi/proxy/audio_input_resource.cc:206: kBitsPerAudioInputSample / 8; On 2014/07/09 20:36:27, piman (slow to review) wrote: > On 2014/07/09 09:39:51, henrika (OOO June 23-July 7) wrote: > > I did run git cl format and it does suggest this indentation. Do you want me > to > > change it anyhow? > > It looks like a bug in git cl format.... line continuations should be +4. Acknowledged.
LGTM, thanks
Many thanks for the fast reply ;-)
The CQ bit was checked by henrika@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/henrika@chromium.org/371273004/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by henrika@chromium.org
Message was sent while issue was closed.
Committed patchset #5 manually as r282357 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
