Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
414643003:
Support configuring the audio buffer duration in the Pepper MediaStream API. (Closed)
|
Created:
6 years, 5 months ago by Anand Mistry (off Chromium) Modified:
6 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionSupport configuring the audio buffer duration in the Pepper MediaStream API.
Increasing the buffer duration allows the Pepper plugin the sleep for
longer between buffers and reduces the per-sample overhead. This all
translates to lower power consumption, at the expense of latency.
BUG=330851
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286774
Patch Set 1 #Patch Set 2 : Tests! #
Total comments: 23
Patch Set 3 : Address review comments #
Total comments: 12
Patch Set 4 : More review comments #
Total comments: 7
Patch Set 5 : Rebase after 417753002 was committed #
Total comments: 2
Patch Set 6 : Add clarifying comment about buffer size. #
Total comments: 2
Messages
Total messages: 20 (0 generated)
cevans@chromium.org: Please review changes in ppapi_messages.h dmichael@chromium.org: Please review changes in... everything else.
+penghuang https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:31: const uint32_t kDefaultDuration = 123; This seems like a weird default... did you benchmark or something? https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:117: base::Time::kMillisecondsPerSecond; Please use CheckedNumeric from base/numerics/safe_math.h for this stuff to avoid potential overflow. https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:127: // TODO(amistry): Fix me! Good catch! File a bug to reference here, please. Unless your intent is to fix it prior to or in this CL, which I'd also support... https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:157: DCHECK_EQ(number_of_frames, audio_params_.frames_per_buffer()); Given this ^^^ Why do you need the new code to allow using partial buffers? Or is this DCHECK wrong now? https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:182: // TODO(amistry): NOT SAFE! Can you elaborate? https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_track_host_base.cc (right): https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_track_host_base.cc:42: // that width != stride in an image buffer. "should not"... do you mean "might not"? https://codereview.chromium.org/414643003/diff/20001/ppapi/tests/test_media_s... File ppapi/tests/test_media_stream_audio_track.cc (right): https://codereview.chromium.org/414643003/diff/20001/ppapi/tests/test_media_s... ppapi/tests/test_media_stream_audio_track.cc:21: // Real max defined in s/max/constants https://codereview.chromium.org/414643003/diff/20001/ppapi/tests/test_media_s... ppapi/tests/test_media_stream_audio_track.cc:212: { kMaxDuration, PP_OK }, // This will take ~30 seconds. We can't have the test take that long. Maybe you can separate that out, and just make sure it can Configure successfully with a buffer that big? https://codereview.chromium.org/414643003/diff/20001/ppapi/tests/test_media_s... ppapi/tests/test_media_stream_audio_track.cc:217: for (size_t i = 0; i < sizeof(durations) / sizeof(durations[0]); ++i) { Instead of duplicating this part, perhaps you can make a function? std::string CheckGetBuffer(int32_t attrib_list[]); (or whatever name you think makes sense) and you would use it like this (just after making attrib_list on 219): ASSERT_SUBTEST_SUCCESS(CheckGetBuffer(attrib_list));
https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:127: // TODO(amistry): Fix me! On 2014/07/23 20:08:41, dmichael wrote: > Good catch! File a bug to reference here, please. > > Unless your intent is to fix it prior to or in this CL, which I'd also > support... I has a CL[1] to fix this issue. With the CL [1], probably this CL need some modifications. Or maybe you could fix this issue within this CL. [1] https://codereview.chromium.org/417753002/
PTAL https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:31: const uint32_t kDefaultDuration = 123; On 2014/07/23 20:08:41, dmichael wrote: > This seems like a weird default... did you benchmark or something? Oops. A hold over from testing. It should be 10, which is the existing behaviour. https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:117: base::Time::kMillisecondsPerSecond; On 2014/07/23 20:08:41, dmichael wrote: > Please use CheckedNumeric from base/numerics/safe_math.h for this stuff to avoid > potential overflow. Done. https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:127: // TODO(amistry): Fix me! On 2014/07/23 20:40:31, Peng wrote: > On 2014/07/23 20:08:41, dmichael wrote: > > Good catch! File a bug to reference here, please. > > > > Unless your intent is to fix it prior to or in this CL, which I'd also > > support... > > I has a CL[1] to fix this issue. With the CL [1], probably this CL need some > modifications. Or maybe you could fix this issue within this CL. > > [1] https://codereview.chromium.org/417753002/ Applied that CL to this. https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:157: DCHECK_EQ(number_of_frames, audio_params_.frames_per_buffer()); On 2014/07/23 20:08:41, dmichael wrote: > Given this ^^^ Why do you need the new code to allow using partial buffers? Or > is this DCHECK wrong now? More like this DCHECK is irrelevant because this code doesn't care what the configured number of frames is. But I think it's still worth leaving here as a check of the API. I believe the audio source is supposed to guarantee this property, and if this number does change, OnSetFormat() should have been called. https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:182: // TODO(amistry): NOT SAFE! On 2014/07/23 20:08:42, dmichael wrote: > Can you elaborate? It was to do with the thread safety of the buffer manager. Not an issue any more. https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_track_host_base.cc (right): https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_track_host_base.cc:42: // that width != stride in an image buffer. On 2014/07/23 20:08:42, dmichael wrote: > "should not"... do you mean "might not"? Yes. Well, and logically, the two should probably be two different things, with the stride being an internal detail. Unless, of course, I'm misreading the API. https://codereview.chromium.org/414643003/diff/20001/ppapi/tests/test_media_s... File ppapi/tests/test_media_stream_audio_track.cc (right): https://codereview.chromium.org/414643003/diff/20001/ppapi/tests/test_media_s... ppapi/tests/test_media_stream_audio_track.cc:21: // Real max defined in On 2014/07/23 20:08:42, dmichael wrote: > s/max/constants Done. https://codereview.chromium.org/414643003/diff/20001/ppapi/tests/test_media_s... ppapi/tests/test_media_stream_audio_track.cc:212: { kMaxDuration, PP_OK }, // This will take ~30 seconds. On 2014/07/23 20:08:42, dmichael wrote: > We can't have the test take that long. > > Maybe you can separate that out, and just make sure it can Configure > successfully with a buffer that big? Done. https://codereview.chromium.org/414643003/diff/20001/ppapi/tests/test_media_s... ppapi/tests/test_media_stream_audio_track.cc:217: for (size_t i = 0; i < sizeof(durations) / sizeof(durations[0]); ++i) { On 2014/07/23 20:08:42, dmichael wrote: > Instead of duplicating this part, perhaps you can make a function? > > std::string CheckGetBuffer(int32_t attrib_list[]); > (or whatever name you think makes sense) > > and you would use it like this (just after making attrib_list on 219): > ASSERT_SUBTEST_SUCCESS(CheckGetBuffer(attrib_list)); Done.
https://codereview.chromium.org/414643003/diff/40001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:184: uint32_t output_buffer_size = 0; Why not use output_buffer_size_ directly? https://codereview.chromium.org/414643003/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:188: output_buffer_size = output_buffer_size_; Moving above three lines into the if (!buffers_.empty()) {} block? https://codereview.chromium.org/414643003/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:193: } else if (active_init_buffers_count_ != init_buffers_count_) { Check (active_init_buffers_count_ != init_buffers_count_) first? if (active_init_buffers_count_ != init_buffers_count_) active_buffer_index_ = -1; if (active_buffer_index_ == -1) { // Try get an active buffer } if (active_buffer_index_ == -1) { // Drop frames. } https://codereview.chromium.org/414643003/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:208: // Next buffer. Not using an active buffer. This comment is confusing. Maybe: The active buffer is new, init the buffer header? https://codereview.chromium.org/414643003/diff/40001/ppapi/tests/test_media_s... File ppapi/tests/test_media_stream_audio_track.cc (right): https://codereview.chromium.org/414643003/diff/40001/ppapi/tests/test_media_s... ppapi/tests/test_media_stream_audio_track.cc:197: std::string TestMediaStreamAudioTrack::TestConfigure() { The test is disabled in https://codereview.chromium.org/410843003/diff/60001/chrome/test/ppapi/ppapi_... . Please enable it. I think the flaky problem should be fixed. https://codereview.chromium.org/414643003/diff/40001/ppapi/tests/test_media_s... ppapi/tests/test_media_stream_audio_track.cc:270: PP_MEDIASTREAMAUDIOTRACK_ATTRIB_BUFFERS, 0, Reset the duration to a small number, otherwise the test could be timeout in buildbot.
https://codereview.chromium.org/414643003/diff/40001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:184: uint32_t output_buffer_size = 0; On 2014/07/24 15:38:37, Peng wrote: > Why not use output_buffer_size_ directly? Done. Artifact of before I synced to your change. https://codereview.chromium.org/414643003/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:188: output_buffer_size = output_buffer_size_; On 2014/07/24 15:38:37, Peng wrote: > Moving above three lines into the if (!buffers_.empty()) {} block? Done. https://codereview.chromium.org/414643003/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:193: } else if (active_init_buffers_count_ != init_buffers_count_) { On 2014/07/24 15:38:37, Peng wrote: > Check (active_init_buffers_count_ != init_buffers_count_) first? > > if (active_init_buffers_count_ != init_buffers_count_) > active_buffer_index_ = -1; > > if (active_buffer_index_ == -1) { > // Try get an active buffer > } > > if (active_buffer_index_ == -1) { > // Drop frames. > } Done. https://codereview.chromium.org/414643003/diff/40001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:208: // Next buffer. Not using an active buffer. On 2014/07/24 15:38:37, Peng wrote: > This comment is confusing. > Maybe: > The active buffer is new, init the buffer header? Done. https://codereview.chromium.org/414643003/diff/40001/ppapi/tests/test_media_s... File ppapi/tests/test_media_stream_audio_track.cc (right): https://codereview.chromium.org/414643003/diff/40001/ppapi/tests/test_media_s... ppapi/tests/test_media_stream_audio_track.cc:197: std::string TestMediaStreamAudioTrack::TestConfigure() { On 2014/07/24 15:38:37, Peng wrote: > The test is disabled in > https://codereview.chromium.org/410843003/diff/60001/chrome/test/ppapi/ppapi_... > . Please enable it. I think the flaky problem should be fixed. Done. https://codereview.chromium.org/414643003/diff/40001/ppapi/tests/test_media_s... ppapi/tests/test_media_stream_audio_track.cc:270: PP_MEDIASTREAMAUDIOTRACK_ATTRIB_BUFFERS, 0, On 2014/07/24 15:38:37, Peng wrote: > Reset the duration to a small number, otherwise the test could be timeout in > buildbot. Done.
lgtm. https://codereview.chromium.org/414643003/diff/60001/ppapi/tests/test_media_s... File ppapi/tests/test_media_stream_audio_track.cc (right): https://codereview.chromium.org/414643003/diff/60001/ppapi/tests/test_media_s... ppapi/tests/test_media_stream_audio_track.cc:238: { kMaxDuration + 1, PP_ERROR_BADARGUMENT }, Seems this test will take more than 10 seconds. I am not sure if it is OK. I don't remember the timeout threshold value. +dmicheal, any comments?
https://codereview.chromium.org/414643003/diff/60001/ppapi/tests/test_media_s... File ppapi/tests/test_media_stream_audio_track.cc (right): https://codereview.chromium.org/414643003/diff/60001/ppapi/tests/test_media_s... ppapi/tests/test_media_stream_audio_track.cc:238: { kMaxDuration + 1, PP_ERROR_BADARGUMENT }, On 2014/07/25 14:16:51, Peng wrote: > Seems this test will take more than 10 seconds. I am not sure if it is OK. I > don't remember the timeout threshold value. I'm not at my desktop, so I can't give you an exact answer, but it's more like 3-4 seconds total (all 3 tests + browser startup). This last case will use a buffer duration of 123ms, since that's the last successful configure. > > +dmicheal, any comments?
PTAL. Rebased after https://codereview.chromium.org/417753002/ was committed.
Sorry for being slow. Looks good overall, but I'm still wondering if we need the added complexity of reading partial buffers... https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:157: DCHECK_EQ(number_of_frames, audio_params_.frames_per_buffer()); On 2014/07/24 01:28:28, Anand Mistry wrote: > On 2014/07/23 20:08:41, dmichael wrote: > > Given this ^^^ Why do you need the new code to allow using partial buffers? Or > > is this DCHECK wrong now? > More like this DCHECK is irrelevant because this code doesn't care what the > configured number of frames is. But I think it's still worth leaving here as a > check of the API. I believe the audio source is supposed to guarantee this > property, and if this number does change, OnSetFormat() should have been called. So if we're guaranteed to have this property, can't we keep the code simpler? Error out of OnData if it doesn't match? Your code looks right, but it's complex, and there's bound to be some subtle bug in it. If we don't need the complexity, we shouldn't add it and have to maintain it. https://codereview.chromium.org/414643003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:134: DCHECK_GT(buffer_size.ValueOrDie(), 0); Can you use IsValid() somewhere before ValidOrDie? Since these numbers can come from the plugin process, it would be nice to fail gracefully if possible. https://codereview.chromium.org/414643003/diff/60001/ppapi/tests/test_media_s... File ppapi/tests/test_media_stream_audio_track.cc (right): https://codereview.chromium.org/414643003/diff/60001/ppapi/tests/test_media_s... ppapi/tests/test_media_stream_audio_track.cc:238: { kMaxDuration + 1, PP_ERROR_BADARGUMENT }, On 2014/07/25 14:16:51, Peng wrote: > Seems this test will take more than 10 seconds. I am not sure if it is OK. I > don't remember the timeout threshold value. > > +dmicheal, any comments? Won't it detect the bad argument without actually waiting on a buffer? Seems it should be fast
https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:157: DCHECK_EQ(number_of_frames, audio_params_.frames_per_buffer()); On 2014/07/29 16:41:24, dmichael wrote: > On 2014/07/24 01:28:28, Anand Mistry wrote: > > On 2014/07/23 20:08:41, dmichael wrote: > > > Given this ^^^ Why do you need the new code to allow using partial buffers? > Or > > > is this DCHECK wrong now? > > More like this DCHECK is irrelevant because this code doesn't care what the > > configured number of frames is. But I think it's still worth leaving here as a > > check of the API. I believe the audio source is supposed to guarantee this > > property, and if this number does change, OnSetFormat() should have been > called. > So if we're guaranteed to have this property, can't we keep the code simpler? > Error out of OnData if it doesn't match? > > Your code looks right, but it's complex, and there's bound to be some subtle bug > in it. If we don't need the complexity, we shouldn't add it and have to maintain > it. The reason this code is complex is to handle cases where the requested buffer duration is not an integer multiple of the received buffer duration. i.e. if the user configures a buffer of 11ms and we get 10ms chunks, the second chuck we receive will span 2 output buffer, and the complexity is pretty much devoted to handling that correctly. https://codereview.chromium.org/414643003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:134: DCHECK_GT(buffer_size.ValueOrDie(), 0); On 2014/07/29 16:41:24, dmichael wrote: > Can you use IsValid() somewhere before ValidOrDie? Since these numbers can come > from the plugin process, it would be nice to fail gracefully if possible. The only number that comes from the plugin, and is used by this calculation, is the buffer duration. And that is bounded to 10,000 by a call to VerifyAttributes(). There'll be an overflow if somehow, our source is >2 channels of 96KHz audio. In which case, we probably have other problems. But more generally, it would be nice if we could return errors from this, back to the plugin. But I'd prefer to do that in a follow-up CL. https://codereview.chromium.org/414643003/diff/60001/ppapi/tests/test_media_s... File ppapi/tests/test_media_stream_audio_track.cc (right): https://codereview.chromium.org/414643003/diff/60001/ppapi/tests/test_media_s... ppapi/tests/test_media_stream_audio_track.cc:238: { kMaxDuration + 1, PP_ERROR_BADARGUMENT }, On 2014/07/29 16:41:24, dmichael wrote: > On 2014/07/25 14:16:51, Peng wrote: > > Seems this test will take more than 10 seconds. I am not sure if it is OK. I > > don't remember the timeout threshold value. > > > > +dmicheal, any comments? > Won't it detect the bad argument without actually waiting on a buffer? Seems it > should be fast Yes. TestConfigure() takes 1.3 seconds in total, as measured on my desktop.
lgtm https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:157: DCHECK_EQ(number_of_frames, audio_params_.frames_per_buffer()); On 2014/07/30 00:22:05, Anand Mistry wrote: > On 2014/07/29 16:41:24, dmichael wrote: > > On 2014/07/24 01:28:28, Anand Mistry wrote: > > > On 2014/07/23 20:08:41, dmichael wrote: > > > > Given this ^^^ Why do you need the new code to allow using partial > buffers? > > Or > > > > is this DCHECK wrong now? > > > More like this DCHECK is irrelevant because this code doesn't care what the > > > configured number of frames is. But I think it's still worth leaving here as > a > > > check of the API. I believe the audio source is supposed to guarantee this > > > property, and if this number does change, OnSetFormat() should have been > > called. > > So if we're guaranteed to have this property, can't we keep the code simpler? > > Error out of OnData if it doesn't match? > > > > Your code looks right, but it's complex, and there's bound to be some subtle > bug > > in it. If we don't need the complexity, we shouldn't add it and have to > maintain > > it. > The reason this code is complex is to handle cases where the requested buffer > duration is not an integer multiple of the received buffer duration. i.e. if the > user configures a buffer of 11ms and we get 10ms chunks, the second chuck we > receive will span 2 output buffer, and the complexity is pretty much devoted to > handling that correctly. So audio_params_.frames_per_buffer() might not reflect what the user configured. I think that's what I was missing. Can you make a comment to clarify that? I.e., buffer->number_of_samples may not be equal to audio_params_.frames_per_buffer(). https://codereview.chromium.org/414643003/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:134: DCHECK_GT(buffer_size.ValueOrDie(), 0); On 2014/07/30 00:22:05, Anand Mistry wrote: > On 2014/07/29 16:41:24, dmichael wrote: > > Can you use IsValid() somewhere before ValidOrDie? Since these numbers can > come > > from the plugin process, it would be nice to fail gracefully if possible. > > The only number that comes from the plugin, and is used by this calculation, is > the buffer duration. And that is bounded to 10,000 by a call to > VerifyAttributes(). There'll be an overflow if somehow, our source is >2 > channels of 96KHz audio. In which case, we probably have other problems. > > But more generally, it would be nice if we could return errors from this, back > to the plugin. But I'd prefer to do that in a follow-up CL. Okay, thanks for explaining. Fine to do in a follow up. https://codereview.chromium.org/414643003/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:253: DCHECK_LE(params.GetBufferDuration().InMilliseconds(), kMinDuration); Did you mean DCHECK_GE? (Does the fact that this was passing imply the audio backend is always using 10ms currently?)
Chris: Can you take a look at ppapi_messages.h https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:157: DCHECK_EQ(number_of_frames, audio_params_.frames_per_buffer()); On 2014/07/30 23:09:22, dmichael wrote: > On 2014/07/30 00:22:05, Anand Mistry wrote: > > On 2014/07/29 16:41:24, dmichael wrote: > > > On 2014/07/24 01:28:28, Anand Mistry wrote: > > > > On 2014/07/23 20:08:41, dmichael wrote: > > > > > Given this ^^^ Why do you need the new code to allow using partial > > buffers? > > > Or > > > > > is this DCHECK wrong now? > > > > More like this DCHECK is irrelevant because this code doesn't care what > the > > > > configured number of frames is. But I think it's still worth leaving here > as > > a > > > > check of the API. I believe the audio source is supposed to guarantee this > > > > property, and if this number does change, OnSetFormat() should have been > > > called. > > > So if we're guaranteed to have this property, can't we keep the code > simpler? > > > Error out of OnData if it doesn't match? > > > > > > Your code looks right, but it's complex, and there's bound to be some subtle > > bug > > > in it. If we don't need the complexity, we shouldn't add it and have to > > maintain > > > it. > > The reason this code is complex is to handle cases where the requested buffer > > duration is not an integer multiple of the received buffer duration. i.e. if > the > > user configures a buffer of 11ms and we get 10ms chunks, the second chuck we > > receive will span 2 output buffer, and the complexity is pretty much devoted > to > > handling that correctly. > So audio_params_.frames_per_buffer() might not reflect what the user configured. > I think that's what I was missing. > > Can you make a comment to clarify that? I.e., buffer->number_of_samples may not > be equal to audio_params_.frames_per_buffer(). Done. https://codereview.chromium.org/414643003/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:253: DCHECK_LE(params.GetBufferDuration().InMilliseconds(), kMinDuration); On 2014/07/30 23:09:22, dmichael wrote: > Did you mean DCHECK_GE? I meant LE. The case I'm catching is where the audio backend gives us 50ms buffers, but we're using 4, 10ms buffers (the default). In which case, every buffer we receive from the backend will drop at least 10ms of samples, by no fault of the user. Arguably, in this case, there's the bigger problem of receiving audio data slower than the user's desire, if they explicitly configured 10ms buffers. At the end of the day, the code will handle this case (and the DCHECK will disappear from the release build) by dropping samples. But that sucks, and I want to catch that (with trybot failures) before this case hits prod. > (Does the fact that this was passing imply the audio backend is always using > 10ms currently?) Yes. We always receive 10ms of 32KHz samples.
tsepez: Can you take a look at ppapi_messages.h?
Messages LGTM
The CQ bit was checked by amistry@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amistry@chromium.org/414643003/100001
Message was sent while issue was closed.
Change committed as 286774
Message was sent while issue was closed.
I think the number of samples is not set correctly anymore. https://codereview.chromium.org/414643003/diff/100001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:217: buffer->number_of_samples = buffer->data_size / bytes_per_frame; Shouldn't this read: buffer->number_of_samples = buffer->data_size * number_of_channels / bytes_per_frame; instead? Now number of samples is set to the number of frames, while in the old code it was set to number_of_frames * number_of_channels
Message was sent while issue was closed.
https://codereview.chromium.org/414643003/diff/100001/content/renderer/pepper... File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right): https://codereview.chromium.org/414643003/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_media_stream_audio_track_host.cc:217: buffer->number_of_samples = buffer->data_size / bytes_per_frame; On 2014/08/15 15:01:23, thembrown wrote: > Shouldn't this read: > buffer->number_of_samples = buffer->data_size * number_of_channels / > bytes_per_frame; > > instead? Now number of samples is set to the number of frames, while in the old > code it was set to number_of_frames * number_of_channels Oops. I'll fix it right away. |
