Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1343273005:
ppapi: add AudioEncode example (Closed)
|
Created:
5 years, 3 months ago by llandwerlin-old Modified:
5 years ago Reviewers:
bbudge CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@nacl-audio-encoder Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionppapi: add AudioEncode example
BUG=461222
Committed: https://crrev.com/862936571c5d894b0a573abf54ef9f5b6109b724
Cr-Commit-Position: refs/heads/master@{#365010}
Patch Set 1 #
Total comments: 28
Patch Set 2 : bbudge's review #
Total comments: 29
Patch Set 3 : #
Total comments: 10
Patch Set 4 : #
Total comments: 6
Patch Set 5 : last nits #
Messages
Total messages: 21 (8 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
lionel.g.landwerlin@intel.com changed reviewers: + bbudge@chromium.org
bbudge@: Here is an example to use the AudioEncoder API. PTAL, thanks.
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... File ppapi/examples/audio_encode/audio_encode.cc (right): https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:39: struct FrameDescription { I'm not sure I see the utility of this struct. You don't really pass these around, so it would be less code to just have 4 member fields on the instance class (i.e. channels_, sample_rate_, sample_size_, and samples_per_frame_.) https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:58: // delete https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:62: void InitializeVideoProfiles(); InitializeAudioProfiles https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:66: void ProbeEncoder(); Maybe a better name is GetProfiles or GetSupportedProfiles? https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:83: void OnBitstreamBuffer(int32_t result, const PP_AudioBitstreamBuffer& buffer); OnGetBitstreamBuffer for consistency? https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:95: bool no_encoding_; A comment explaining what this is for? https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:226: void AudioEncoderInstance::OnGetFirstBuffer(int32_t result, This is very similar to OnGetBuffer. Could you use a sentinel to detect when you need to initialize frame_description? This could be a boolean (e.g. got_first_buffer_) or you could detect an unitialized FrameDescription (e.g. frame_description_.channels == 0). https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:295: &AudioEncoderInstance::OnAudioTrackReconfigured)); OnAudioTrackConfigured seems like a better name here. https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:303: return; Is it necessary to handle this differently than other errors here? https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:313: &AudioEncoderInstance::OnGetBuffer)); nit: It seems clearer and more consistent with encoder flow to request the audio buffer before the bitstream buffer. https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:335: if (buffer.GetDataBufferSize() != track_buffer.GetDataBufferSize()) { This seems like a good defensive check, but isn't this a pretty serious failure, and shouldn't we stop encoding in this case? https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:346: &AudioEncoderInstance::OnEncodeDone)); I think you could eliminate a lot of code duplication in this method by not trying to return early in the error case. Like this: if (buffer.GetDataBufferSize() != track_buffer.GetDataBufferSize()) result = PP_ERROR_FAILED; if (result == PP_OK) { memcpy... audio_encoder_.Encode... } else { LogError... } audio_track_.RecycleBuffer(track_buffer); If you want the special error message, you could introduce a char* error_message local and set it appropriately.
https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... File ppapi/examples/audio_encode/audio_encode.cc (right): https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:39: struct FrameDescription { On 2015/11/24 19:20:58, bbudge(OOO until 11-24) wrote: > I'm not sure I see the utility of this struct. You don't really pass these > around, so it would be less code to just have 4 member fields on the instance > class (i.e. channels_, sample_rate_, sample_size_, and samples_per_frame_.) Done. https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:58: // On 2015/11/24 19:20:58, bbudge(OOO until 11-24) wrote: > delete Done. https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:62: void InitializeVideoProfiles(); On 2015/11/24 19:20:58, bbudge(OOO until 11-24) wrote: > InitializeAudioProfiles Done. https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:66: void ProbeEncoder(); On 2015/11/24 19:20:58, bbudge(OOO until 11-24) wrote: > Maybe a better name is GetProfiles or GetSupportedProfiles? Done. https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:83: void OnBitstreamBuffer(int32_t result, const PP_AudioBitstreamBuffer& buffer); On 2015/11/24 19:20:58, bbudge(OOO until 11-24) wrote: > OnGetBitstreamBuffer for consistency? Done. https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:95: bool no_encoding_; On 2015/11/24 19:20:59, bbudge(OOO until 11-24) wrote: > A comment explaining what this is for? Done. https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:226: void AudioEncoderInstance::OnGetFirstBuffer(int32_t result, On 2015/11/24 19:20:59, bbudge(OOO until 11-24) wrote: > This is very similar to OnGetBuffer. Could you use a sentinel to detect when you > need to initialize frame_description? This could be a boolean (e.g. > got_first_buffer_) or you could detect an unitialized FrameDescription (e.g. > frame_description_.channels == 0). Done. https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:295: &AudioEncoderInstance::OnAudioTrackReconfigured)); On 2015/11/24 19:20:58, bbudge(OOO until 11-24) wrote: > OnAudioTrackConfigured seems like a better name here. Done. https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:303: return; On 2015/11/24 19:20:59, bbudge(OOO until 11-24) wrote: > Is it necessary to handle this differently than other errors here? I'm not sure what you think is different from the other errors here. https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:313: &AudioEncoderInstance::OnGetBuffer)); On 2015/11/24 19:20:58, bbudge(OOO until 11-24) wrote: > nit: It seems clearer and more consistent with encoder flow to request the audio > buffer before the bitstream buffer. Done. https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:335: if (buffer.GetDataBufferSize() != track_buffer.GetDataBufferSize()) { On 2015/11/24 19:20:59, bbudge(OOO until 11-24) wrote: > This seems like a good defensive check, but isn't this a pretty serious failure, > and shouldn't we stop encoding in this case? Done. https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:346: &AudioEncoderInstance::OnEncodeDone)); On 2015/11/24 19:20:58, bbudge(OOO until 11-24) wrote: > I think you could eliminate a lot of code duplication in this method by not > trying to return early in the error case. Like this: > > if (buffer.GetDataBufferSize() != track_buffer.GetDataBufferSize()) > result = PP_ERROR_FAILED; > > if (result == PP_OK) { > memcpy... > audio_encoder_.Encode... > } else { > LogError... > } > > audio_track_.RecycleBuffer(track_buffer); > > If you want the special error message, you could introduce a char* error_message > local and set it appropriately. > > Done.
Ping?
https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... File ppapi/examples/audio_encode/audio_encode.cc (right): https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:253: return; see comment below. https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:303: return; On 2015/11/25 18:11:46, llandwerlin wrote: > On 2015/11/24 19:20:59, bbudge(OOO until 11-24) wrote: > > Is it necessary to handle this differently than other errors here? > > I'm not sure what you think is different from the other errors here. This is from the MediaStreamAudioTrack. Does it normally return PP_ERROR_ABORTED like audio/video decoder/encoder resources can when Closing? Normally Pepper resources only abort when they are destroyed. Same thing for the first lines of AudioEncoderInstance::OnGetBuffer. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... File ppapi/examples/audio_encode/audio_encode.cc (right): https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:9: #include <limits> Is <limits> used? #include <map> #include <string> (include what you use) https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:54: void OnGetFirstBuffer(int32_t result, pp::AudioBuffer buffer); delete. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:81: bool is_encoding_; This seems to be read-only - you initialize and set it but never test it. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:132: if (profile == "wav") match bracketing style of 'else' part. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:172: audio_encoder_ = pp::AudioEncoder(this); Initialize in constructor. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:215: audio_track_.Close(); Would it be safer and clearer to Close the audio track first? https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:219: void AudioEncoderInstance::OnGetFirstBuffer(int32_t result, This method isn't called. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:231: return; This can be handled by next 'if'. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:250: InitializeEncoder(); Is there data in the first buffer? If so, you might want to pass it on to InitializeEncoder before recycling it. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:259: GetEncoderBuffer(buffer); match bracketing style of 'if' part. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:267: audio_encoder_ = pp::AudioEncoder(this); Initialize in constructor. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:278: return; This seems like an error that should be logged. Can the following 'if' handle this? https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:324: audio_track_.RecycleBuffer(track_buffer); As this stands, you might recycle this buffer twice.
https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... File ppapi/examples/audio_encode/audio_encode.cc (right): https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:253: return; On 2015/12/04 00:59:12, bbudge wrote: > see comment below. This is to avoid to report an error that isn't one. This was just aborted by the user. https://codereview.chromium.org/1343273005/diff/80001/ppapi/examples/audio_en... ppapi/examples/audio_encode/audio_encode.cc:303: return; On 2015/12/04 00:59:12, bbudge wrote: > On 2015/11/25 18:11:46, llandwerlin wrote: > > On 2015/11/24 19:20:59, bbudge(OOO until 11-24) wrote: > > > Is it necessary to handle this differently than other errors here? > > > > I'm not sure what you think is different from the other errors here. > > This is from the MediaStreamAudioTrack. Does it normally return PP_ERROR_ABORTED > like audio/video decoder/encoder resources can when Closing? Normally Pepper > resources only abort when they are destroyed. > > Same thing for the first lines of AudioEncoderInstance::OnGetBuffer. I get it now. I want to avoid to display an error in the aborted case because it's triggered by the user when the encoding/recording stops. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... File ppapi/examples/audio_encode/audio_encode.cc (right): https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:9: #include <limits> On 2015/12/04 00:59:13, bbudge wrote: > Is <limits> used? > > #include <map> > #include <string> > (include what you use) Done. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:54: void OnGetFirstBuffer(int32_t result, pp::AudioBuffer buffer); On 2015/12/04 00:59:12, bbudge wrote: > delete. Done. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:81: bool is_encoding_; On 2015/12/04 00:59:13, bbudge wrote: > This seems to be read-only - you initialize and set it but never test it. Removing as there is only one codec supported for now. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:132: if (profile == "wav") On 2015/12/04 00:59:13, bbudge wrote: > match bracketing style of 'else' part. Done. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:172: audio_encoder_ = pp::AudioEncoder(this); On 2015/12/04 00:59:12, bbudge wrote: > Initialize in constructor. Done. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:215: audio_track_.Close(); On 2015/12/04 00:59:12, bbudge wrote: > Would it be safer and clearer to Close the audio track first? Done. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:219: void AudioEncoderInstance::OnGetFirstBuffer(int32_t result, On 2015/12/04 00:59:13, bbudge wrote: > This method isn't called. Sorry, done. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:231: return; On 2015/12/04 00:59:13, bbudge wrote: > This can be handled by next 'if'. The point is to avoid a warning because aborted means stopped by the user. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:250: InitializeEncoder(); On 2015/12/04 00:59:13, bbudge wrote: > Is there data in the first buffer? If so, you might want to pass it on to > InitializeEncoder before recycling it. Indeed this drops the first audio buffer (first 10ms), but this makes the code a bit easier to read, since we might have some reconfiguration to do before we can submit the buffers to the audio encoder. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:259: GetEncoderBuffer(buffer); On 2015/12/04 00:59:12, bbudge wrote: > match bracketing style of 'if' part. Done. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:267: audio_encoder_ = pp::AudioEncoder(this); On 2015/12/04 00:59:12, bbudge wrote: > Initialize in constructor. I think this is required because once the encoder is close, it's unusable. We need to create a new one. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:278: return; On 2015/12/04 00:59:13, bbudge wrote: > This seems like an error that should be logged. Can the following 'if' handle > this? Again, this is related to a user's action. I think we shouldn't display any error, but let me know if you prefer an actual error. https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:324: audio_track_.RecycleBuffer(track_buffer); On 2015/12/04 00:59:12, bbudge wrote: > As this stands, you might recycle this buffer twice. Whoops, my mistake.
https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... File ppapi/examples/audio_encode/audio_encode.cc (right): https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:250: InitializeEncoder(); On 2015/12/08 12:29:23, llandwerlin wrote: > On 2015/12/04 00:59:13, bbudge wrote: > > Is there data in the first buffer? If so, you might want to pass it on to > > InitializeEncoder before recycling it. > > Indeed this drops the first audio buffer (first 10ms), but this makes the code a > bit easier to read, since we might have some reconfiguration to do before we can > submit the buffers to the audio encoder. OK https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:267: audio_encoder_ = pp::AudioEncoder(this); On 2015/12/08 12:29:23, llandwerlin wrote: > On 2015/12/04 00:59:12, bbudge wrote: > > Initialize in constructor. > > I think this is required because once the encoder is close, it's unusable. We > need to create a new one. OK, I guess the same logic would apply to the audio_track_ too? https://codereview.chromium.org/1343273005/diff/100001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:278: return; On 2015/12/08 12:29:23, llandwerlin wrote: > On 2015/12/04 00:59:13, bbudge wrote: > > This seems like an error that should be logged. Can the following 'if' handle > > this? > > Again, this is related to a user's action. I think we shouldn't display any > error, but let me know if you prefer an actual error. Right, just catch all errors below. https://codereview.chromium.org/1343273005/diff/120001/ppapi/examples/audio_e... File ppapi/examples/audio_encode/audio_encode.cc (right): https://codereview.chromium.org/1343273005/diff/120001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:80: bool is_encoding_; This bit of state doesn't seem to be needed. https://codereview.chromium.org/1343273005/diff/120001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:90: typedef std::map<std::string, PP_AudioProfile> AudioProfileFromStringMap; Of all of this, only ProfileToString seems to be needed. I think you can eliminate these types, and the ProfileFromString and ProfileToString methods, and just use a switch in OnGetSupportedProfiles to convert profiles to strings. https://codereview.chromium.org/1343273005/diff/120001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:106: audio_encoder_(this) { I now see why you didn't do this before. It would be better not to have to create a throw-away encoder just to support GetSupportedProfiles. Could you just initialize to a null resource here and defer the call to GetSupportedProfiles until after the audio_track_ has given you a buffer? Also, I don't see how the results of GetSupportedProfiles are used. Are you assuming OPUS is always supported? https://codereview.chromium.org/1343273005/diff/120001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:200: void AudioEncoderInstance::StartAudioTrack(const pp::Resource& resource_track) { Log("StartAudioTrack!"); ? https://codereview.chromium.org/1343273005/diff/120001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:201: channels_ = 0; It would be more robust if channels_ was always 0 when the audio_track_ was not running, so this should be done in StopAudioTrack.
https://codereview.chromium.org/1343273005/diff/120001/ppapi/examples/audio_e... File ppapi/examples/audio_encode/audio_encode.cc (right): https://codereview.chromium.org/1343273005/diff/120001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:80: bool is_encoding_; On 2015/12/08 17:12:30, bbudge wrote: > This bit of state doesn't seem to be needed. Done. https://codereview.chromium.org/1343273005/diff/120001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:90: typedef std::map<std::string, PP_AudioProfile> AudioProfileFromStringMap; On 2015/12/08 17:12:30, bbudge wrote: > Of all of this, only ProfileToString seems to be needed. I think you can > eliminate these types, and the ProfileFromString and ProfileToString methods, > and just use a switch in OnGetSupportedProfiles to convert profiles to strings. We forward all the supported profiles to JS. Then once the UI has selected what codec is used, it sends back the encoder configuration with the codec name in a string. That's why I'm using this table. I agree this is a bit silly because there is only one codec available. Changing to if statements. https://codereview.chromium.org/1343273005/diff/120001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:106: audio_encoder_(this) { On 2015/12/08 17:12:30, bbudge wrote: > I now see why you didn't do this before. It would be better not to have to > create a throw-away encoder just to support GetSupportedProfiles. Could you just > initialize to a null resource here and defer the call to GetSupportedProfiles > until after the audio_track_ has given you a buffer? > > Also, I don't see how the results of GetSupportedProfiles are used. Are you > assuming OPUS is always supported? Done for the encoder initialized at null here. The results of GetSupportedProfiles are propagated to JS, to populate the UI with the available options. https://codereview.chromium.org/1343273005/diff/120001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:200: void AudioEncoderInstance::StartAudioTrack(const pp::Resource& resource_track) { On 2015/12/08 17:12:30, bbudge wrote: > Log("StartAudioTrack!"); ? Dropping the traces. https://codereview.chromium.org/1343273005/diff/120001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:201: channels_ = 0; On 2015/12/08 17:12:30, bbudge wrote: > It would be more robust if channels_ was always 0 when the audio_track_ was not > running, so this should be done in StopAudioTrack. Done.
A couple of comments, otherwise LGTM https://codereview.chromium.org/1343273005/diff/140001/ppapi/examples/audio_e... File ppapi/examples/audio_encode/audio_encode.cc (right): https://codereview.chromium.org/1343273005/diff/140001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:94: samples_per_frame_(0) { GetSupportedProfiles needs the audio_encoder_ to be non-null. https://codereview.chromium.org/1343273005/diff/140001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:206: audio_track_.RecycleBuffer(buffer); Maybe a comment stating that we discard the first buffer, and why? https://codereview.chromium.org/1343273005/diff/140001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:221: audio_encoder_ = pp::AudioEncoder(this); I guess you have to initialize this earlier, before GetSupportedProfiles(), and then test here for null resource before optionally creating another one.
Thanks! https://codereview.chromium.org/1343273005/diff/140001/ppapi/examples/audio_e... File ppapi/examples/audio_encode/audio_encode.cc (right): https://codereview.chromium.org/1343273005/diff/140001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:94: samples_per_frame_(0) { On 2015/12/10 20:28:35, bbudge wrote: > GetSupportedProfiles needs the audio_encoder_ to be non-null. Done. https://codereview.chromium.org/1343273005/diff/140001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:206: audio_track_.RecycleBuffer(buffer); On 2015/12/10 20:28:35, bbudge wrote: > Maybe a comment stating that we discard the first buffer, and why? Done. https://codereview.chromium.org/1343273005/diff/140001/ppapi/examples/audio_e... ppapi/examples/audio_encode/audio_encode.cc:221: audio_encoder_ = pp::AudioEncoder(this); On 2015/12/10 20:28:35, bbudge wrote: > I guess you have to initialize this earlier, before GetSupportedProfiles(), and > then test here for null resource before optionally creating another one. Done.
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 bbudge@chromium.org Link to the patchset: https://codereview.chromium.org/1343273005/#ps160001 (title: "last nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1343273005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1343273005/160001
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== ppapi: add AudioEncode example BUG=461222 ========== to ========== ppapi: add AudioEncode example BUG=461222 Committed: https://crrev.com/862936571c5d894b0a573abf54ef9f5b6109b724 Cr-Commit-Position: refs/heads/master@{#365010} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/862936571c5d894b0a573abf54ef9f5b6109b724 Cr-Commit-Position: refs/heads/master@{#365010} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
