Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1302423006:
Ensure that data is not overwritten in the audio input shared memory ring buffer that sits on the b… (Closed)
|
Created:
5 years, 3 months ago by Henrik Grunell Modified:
5 years, 3 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure that data is not overwritten in the audio input shared memory ring buffer that sits on the browser/renderer boundary.
This is for "low-latency" audio input streams only.
* The reading side (renderer) signals back to the writing side over the socket that it has read a buffer.
* The writing side receives the verifications and checks that there is room in the ring buffer before writing so that no data is overwritten.
* If the ring buffer is full, log error and drop the data.
* Report UMA stats of how many drops we had when the audio input stream is destroyed.
* Make SyncSocket::Peek virtual to be able to override in test mocks.
* Some refactoring of AudioInputSyncWriter to be able to test it.
* Add a new unit test for AudioInputSyncWriter.
BUG=523224
NOTRY=true
Committed: https://crrev.com/8d9071da52c70d300bfc0cdc0448c564b39764f4
Cr-Commit-Position: refs/heads/master@{#348156}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Code review + rebase. #
Total comments: 7
Patch Set 3 : Code review and rebase. Added unit test, and refactorings for that. #
Total comments: 6
Patch Set 4 : Code review. Unit test improvement. Changed read verification algorithm. #
Total comments: 12
Patch Set 5 : Code review. #
Total comments: 6
Patch Set 6 : Code review. #
Total comments: 2
Patch Set 7 : Code review. Compile fixes. Updated comment in sync_socket.h. #Patch Set 8 : Rebase #Patch Set 9 : Tracking down compile errors. #
Total comments: 1
Patch Set 10 : Fixed aligment of data buffer in unit test. Some other Win compile fixes. Rebase. #Patch Set 11 : Fixed log call expectations for Android in unit test. Rebase. #Messages
Total messages: 47 (10 generated)
grunell@chromium.org changed reviewers: + dalecurtis@chromium.org, henrika@chromium.org, tommi@chromium.org
There's two "TODO BEFORE COMMIT" in the code, input is welcome.
I'm heading out the door right now, will take a look later. Could we do a unit test for this?
On 2015/08/27 15:19:47, tommi wrote: > I'm heading out the door right now, will take a look later. Could we do a unit > test for this? I'll look at that tomorrow.
Did you consider just using the output system as is with the modification that you only give a non-zero timeout to RecieveWithTimeout when you're more than n/2 buffers behind? Do we care if it's n/2? Can't we just track available unread slots then? This allows the comment in AudioDeviceThread to stay mostly the same with a small caveat about the browser side only waiting when the renderer falls too far behind.
https://codereview.chromium.org/1302423006/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/1302423006/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_sync_writer.cc:59: // TODO BEFORE COMMIT: Maybe percentage makes more sense here? For consistency please use the same pattern as we do on output.
Nice work Henrik. Have you tested this scheme under high load and what is the results today using the 10ms timeout? https://codereview.chromium.org/1302423006/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/1302423006/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_sync_writer.cc:20: const base::TimeDelta kReadCheckTimeout = Have you experimented with high load to see the effect of reducing this value? Might also differ between OS:es. https://codereview.chromium.org/1302423006/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_sync_writer.cc:101: if (expected_buffer_reads_ == Did you check this scheme for a segment count equal to 1? https://codereview.chromium.org/1302423006/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1302423006/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:16501: + The number of input audio read verifications that missed deadline. The read "missed deadline". Can it be rewritten? "Missed a deadline set to.." or something more specific.
On 2015/08/27 16:43:10, DaleCurtis wrote: > Did you consider just using the output system as is with the modification that > you only give a non-zero timeout to RecieveWithTimeout when you're more than n/2 > buffers behind? Do we care if it's n/2? Can't we just track available unread > slots then? RecieveWithTimeout doesn't allow zero timeout. We could use a very small value, but I don't see a guarantee in the posix code that it will try at least once, so that's not robust. n/2 is just a picked value, no science behind that. Not sure what you mean with "Can't we just track available unread slots then?" > This allows the comment in AudioDeviceThread to stay mostly the same with a > small caveat about the browser side only waiting when the renderer falls too far > behind. I agree it would be good to converge this. (Or perhaps split into base, input and output classes, with only Run() that differs.)
https://codereview.chromium.org/1302423006/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/1302423006/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_sync_writer.cc:20: const base::TimeDelta kReadCheckTimeout = On 2015/08/28 07:56:25, henrika wrote: > Have you experimented with high load to see the effect of reducing this value? > Might also differ between OS:es. I've stress tested on my machine, couldn't provoke any timeout at all. It can indeed differ. I think we'll just have to choose a reasonable value. https://codereview.chromium.org/1302423006/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_sync_writer.cc:59: // TODO BEFORE COMMIT: Maybe percentage makes more sense here? On 2015/08/27 16:43:39, DaleCurtis wrote: > For consistency please use the same pattern as we do on output. Done. https://codereview.chromium.org/1302423006/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_sync_writer.cc:101: if (expected_buffer_reads_ == On 2015/08/28 07:56:25, henrika wrote: > Did you check this scheme for a segment count equal to 1? Good point. I changed the for loop condition to shared_memory_segment_count_ + 1 / 2 to round up so that size 1 also reads once. Tested that manually, works fine. https://codereview.chromium.org/1302423006/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1302423006/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:16501: + The number of input audio read verifications that missed deadline. The read On 2015/08/28 07:56:25, henrika wrote: > "missed deadline". Can it be rewritten? "Missed a deadline set to.." or > something more specific. Tried to clarify this.
looks good - would be great to see some sort of a test https://codereview.chromium.org/1302423006/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/1302423006/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:21: // blocking the soundcard thread and wait before dropping data. Could this be dynamically chosen somehow based on the buffer size in use? Bigger buffers would then have more resiliency which would be naturally expected. https://codereview.chromium.org/1302423006/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:133: for (uint32 i = 0; i < (shared_memory_segment_count_ + 1) / 2; ++i) { nit: calculate nr or repetitions outside the loop https://codereview.chromium.org/1302423006/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:137: if (bytes_received != sizeof(dummy)) { if we timeout, should we break from the loop? Btw, could we use the value we receive to verify the expected read state too? https://codereview.chromium.org/1302423006/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.h (right): https://codereview.chromium.org/1302423006/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.h:77: // timeouts when verifying. is it possible to point to some design? As is, it's hard to understand what this actually means. It would be good actually to document the design in a comment in this header.
On 2015/08/28 11:18:39, Henrik Grunell wrote: > On 2015/08/27 16:43:10, DaleCurtis wrote: > > Did you consider just using the output system as is with the modification that > > you only give a non-zero timeout to RecieveWithTimeout when you're more than > n/2 > > buffers behind? Do we care if it's n/2? Can't we just track available unread > > slots then? > > RecieveWithTimeout doesn't allow zero timeout. We could use a very small value, > but I don't see a guarantee in the posix code that it will try at least once, so > that's not robust. Ah I forgot about that. You can call SyncSocket::Peek() to see if there's data available for reading if you want to go with the describe approace > > n/2 is just a picked value, no science behind that. > > Not sure what you mean with "Can't we just track available unread slots then?" I mean that you don't really care if the renderer is n/2 buffers behind, we only care that we're about to stomp a buffer which has not previously been read -- so instead of checking every n/2 buffers, we could just check when we know there are no more available slots. > > > This allows the comment in AudioDeviceThread to stay mostly the same with a > > small caveat about the browser side only waiting when the renderer falls too > far > > behind. > > I agree it would be good to converge this. (Or perhaps split into base, input > and output classes, with only Run() that differs.)
On 2015/08/28 16:39:21, DaleCurtis wrote: > On 2015/08/28 11:18:39, Henrik Grunell wrote: > > On 2015/08/27 16:43:10, DaleCurtis wrote: > > > Did you consider just using the output system as is with the modification > that > > > you only give a non-zero timeout to RecieveWithTimeout when you're more than > > n/2 > > > buffers behind? Do we care if it's n/2? Can't we just track available unread > > > slots then? > > > > RecieveWithTimeout doesn't allow zero timeout. We could use a very small > value, > > but I don't see a guarantee in the posix code that it will try at least once, > so > > that's not robust. > > Ah I forgot about that. You can call SyncSocket::Peek() to see if there's data > available for reading if you want to go with the describe approace Sorry, I don't understand the algorithm you're proposing. > > > > > n/2 is just a picked value, no science behind that. > > > > Not sure what you mean with "Can't we just track available unread slots then?" > > I mean that you don't really care if the renderer is n/2 buffers behind, we only > care that we're about to stomp a buffer which has not previously been read -- so > instead of checking every n/2 buffers, we could just check when we know there > are no more available slots. That makes sense. I removed the loop, and just check once since that's what we need to be allowed to write. > > > > > > This allows the comment in AudioDeviceThread to stay mostly the same with a > > > small caveat about the browser side only waiting when the renderer falls too > > far > > > behind. > > > > I agree it would be good to converge this. (Or perhaps split into base, input > > and output classes, with only Run() that differs.)
https://codereview.chromium.org/1302423006/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/1302423006/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:21: // blocking the soundcard thread and wait before dropping data. On 2015/08/28 13:43:06, tommi wrote: > Could this be dynamically chosen somehow based on the buffer size in use? > Bigger buffers would then have more resiliency which would be naturally > expected. I assume you mean number of segments in the ring buffer. I'm not sure that would make things better for us. I don't see the benefits. Despite that, what value should we have here? My gut feeling tells me 10 ms is a reasonable value - the usual size of each data chunk and average interval between each processing round. https://codereview.chromium.org/1302423006/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:133: for (uint32 i = 0; i < (shared_memory_segment_count_ + 1) / 2; ++i) { On 2015/08/28 13:43:06, tommi wrote: > nit: calculate nr or repetitions outside the loop I've removed the loop. I think Dale has a point, we could simply just check once when wee need. https://codereview.chromium.org/1302423006/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:137: if (bytes_received != sizeof(dummy)) { On 2015/08/28 13:43:06, tommi wrote: > if we timeout, should we break from the loop? Loop is gone. > Btw, could we use the value we receive to verify the expected read state too? Yes, that's a good point. Done.
https://codereview.chromium.org/1302423006/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/1302423006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:21: const base::TimeDelta kReadCheckTimeout = This is a static initializer, you'll need to keep this as a primitive type and convert to a timedelta inside a function. As for the timeout itself, can you just use the same one we use in the reader? duration / 2? https://codereview.chromium.org/1302423006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:134: bytes_received = socket_->ReceiveWithTimeout( This will work, but I suspect during periods of high load it'll take longer for the user to get back to a glitch free experience. With this if 10 slots get filled, you'll only be working with 9 for the rest of the session -- so you have no buffer for future periods of load. I think it's still worth having the while loop to speed up recovery, you just want to make sure that you decrease timeout according to the time taken by each receive (just like is done in the AUdioSyncReader). https://codereview.chromium.org/1302423006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:143: } else if (index_read != next_read_buffer_index_) { Hmm, this should never happen right? I think this should be a DCHECK/CHECK instead then.
grunell@chromium.org changed reviewers: + thestig@chromium.org
thestig@: please review base/sync_socket.h Tommi and Dale: please review. Code review fixes. Unit test improvement. Changed read verification algorithm. https://codereview.chromium.org/1302423006/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/1302423006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:21: const base::TimeDelta kReadCheckTimeout = On 2015/09/02 16:18:50, DaleCurtis wrote: > This is a static initializer, you'll need to keep this as a primitive type and > convert to a timedelta inside a function. > > As for the timeout itself, can you just use the same one we use in the reader? > duration / 2? I've removed this and changed the verification algorithm. https://codereview.chromium.org/1302423006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:134: bytes_received = socket_->ReceiveWithTimeout( On 2015/09/02 16:18:50, DaleCurtis wrote: > This will work, but I suspect during periods of high load it'll take longer for > the user to get back to a glitch free experience. With this if 10 slots get > filled, you'll only be working with 9 for the rest of the session -- so you have > no buffer for future periods of load. > > I think it's still worth having the while loop to speed up recovery, you just > want to make sure that you decrease timeout according to the time taken by each > receive (just like is done in the AUdioSyncReader). It can't really work like on the output side since both are driven form the browser process side. (I.e. consumer for output and producer for input.) I did rewrite to instead receive as much data as possible, but peek first to avoid timeout altogether. Then simply check if the ring buffer is full or not. Since we have a buffer already, of 100 ms as of now, having a timeout is redundant really. If the ring buffer is full we have already "waited" the last 9 writes, no point waiting any more if the read side hasn't been able to read. https://codereview.chromium.org/1302423006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:143: } else if (index_read != next_read_buffer_index_) { On 2015/09/02 16:18:50, DaleCurtis wrote: > Hmm, this should never happen right? I think this should be a DCHECK/CHECK > instead then. Right, done.
https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:121: ++write_count_; here we increment write_count_ without having actually written anything. should this be done below? https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:129: size_t bytes_received = socket_->ReceiveWithTimeout( Is it possible to read everything that's in the buffer in one call? scoped_ptr<int> indices(new int[<segment count>]); size_t bytes_received = socket_->ReceiveWithTimeout(&indices[0], <count * sizeof(indices[0])>, ...); <loop through and check all received indices> number_of_filled_segments_ -= bytes_received / sizeof(indices[0]); etc. https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:132: // TODO(grunell): This could happen if this thread has bad luck and don't nit: ...and don't -> ...and doesn't https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:133: // get to run before the timeout dealine. Otherwise it should never deadline https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.h (right): https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.h:58: uint8* shared_memory_; I wonder if we should just make the AudioInputSyncWriter own the shared memory while it's exclusively writing to it... perhaps something to think about for the next cl ;)
https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:121: ++write_count_; On 2015/09/03 14:55:13, tommi wrote: > here we increment write_count_ without having actually written anything. should > this be done below? It actually counts Write() calls, attempted write. Moved up to top of function to clarify that + clarified the comment in header. https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:129: size_t bytes_received = socket_->ReceiveWithTimeout( On 2015/09/03 14:55:13, tommi wrote: > Is it possible to read everything that's in the buffer in one call? > > scoped_ptr<int> indices(new int[<segment count>]); > size_t bytes_received = socket_->ReceiveWithTimeout(&indices[0], <count * > sizeof(indices[0])>, ...); > <loop through and check all received indices> > > number_of_filled_segments_ -= bytes_received / sizeof(indices[0]); > etc. That would mean that we have to timeout every time we call ReceiveWithTimeout, which is not good. And we probably don't want to rely on what Peek() tells us is available based on the caveat in the comment for it. https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:132: // TODO(grunell): This could happen if this thread has bad luck and don't On 2015/09/03 14:55:13, tommi wrote: > nit: ...and don't -> ...and doesn't Done. https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:133: // get to run before the timeout dealine. Otherwise it should never On 2015/09/03 14:55:13, tommi wrote: > deadline Done. https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.h (right): https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.h:58: uint8* shared_memory_; On 2015/09/03 14:55:13, tommi wrote: > I wonder if we should just make the AudioInputSyncWriter own the shared memory > while it's exclusively writing to it... perhaps something to think about for the > next cl ;) Yes, that might be a good idea. (Don't know if there's a reason not to.)
https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:129: size_t bytes_received = socket_->ReceiveWithTimeout( On 2015/09/03 15:29:51, Henrik Grunell wrote: > On 2015/09/03 14:55:13, tommi wrote: > > Is it possible to read everything that's in the buffer in one call? > > > > scoped_ptr<int> indices(new int[<segment count>]); > > size_t bytes_received = socket_->ReceiveWithTimeout(&indices[0], <count * > > sizeof(indices[0])>, ...); > > <loop through and check all received indices> > > > > number_of_filled_segments_ -= bytes_received / sizeof(indices[0]); > > etc. > > That would mean that we have to timeout every time we call ReceiveWithTimeout, > which is not good. And we probably don't want to rely on what Peek() tells us is > available based on the caveat in the comment for it. We can rely on what Peek() tells us as long as we treat it as a bool (as you're doing already). I wasn't suggesting to skip the call to Peek() btw, so this approach would be at worst equally costly as the current approach, and at best, less so, since it involves fewer context switches or user/kernel mode switches.
https://codereview.chromium.org/1302423006/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/1302423006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:125: while (true) { If you don't want to use tommi@'s suggestion to read all at once, you can change this to while (socket_->Peek() >= sizeof(uint32)) {} https://codereview.chromium.org/1302423006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:130: size_t bytes_received = socket_->ReceiveWithTimeout( Just use Receive() if you're only going to read what's in Peek(). https://codereview.chromium.org/1302423006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:132: if (bytes_received != sizeof(index_read)) { Can make this a dcheck if you take my suggestion above.
base/ lgtm
grunell@chromium.org changed reviewers: + jwd@chromium.org
jwd@: please review tools/metrics/histograms/histograms.xml Tommi: ptal. https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/1302423006/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:129: size_t bytes_received = socket_->ReceiveWithTimeout( On 2015/09/03 15:47:52, tommi wrote: > On 2015/09/03 15:29:51, Henrik Grunell wrote: > > On 2015/09/03 14:55:13, tommi wrote: > > > Is it possible to read everything that's in the buffer in one call? > > > > > > scoped_ptr<int> indices(new int[<segment count>]); > > > size_t bytes_received = socket_->ReceiveWithTimeout(&indices[0], <count * > > > sizeof(indices[0])>, ...); > > > <loop through and check all received indices> > > > > > > number_of_filled_segments_ -= bytes_received / sizeof(indices[0]); > > > etc. > > > > That would mean that we have to timeout every time we call ReceiveWithTimeout, > > which is not good. And we probably don't want to rely on what Peek() tells us > is > > available based on the caveat in the comment for it. > > We can rely on what Peek() tells us as long as we treat it as a bool (as you're > doing already). I wasn't suggesting to skip the call to Peek() btw, so this > approach would be at worst equally costly as the current approach, and at best, > less so, since it involves fewer context switches or user/kernel mode switches. Tried this, but both Receive and ReceiveWTO blocks. So unfortunately it doesn't work. https://codereview.chromium.org/1302423006/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/1302423006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:125: while (true) { On 2015/09/03 16:50:40, DaleCurtis wrote: > If you don't want to use tommi@'s suggestion to read all at once, you can change > this to while (socket_->Peek() >= sizeof(uint32)) {} From comment on Peek(): "NOTE: Some implementations cannot reliably determine the number of bytes available so avoid using the returned size as a promise and simply test against zero." Feels safer to just test > 0. (Side note: Peek should really return bool if the value should be used as such.) https://codereview.chromium.org/1302423006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:130: size_t bytes_received = socket_->ReceiveWithTimeout( On 2015/09/03 16:50:40, DaleCurtis wrote: > Just use Receive() if you're only going to read what's in Peek(). Need a timeout since Peek return value isn't reliable. https://codereview.chromium.org/1302423006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_sync_writer.cc:132: if (bytes_received != sizeof(index_read)) { On 2015/09/03 16:50:40, DaleCurtis wrote: > Can make this a dcheck if you take my suggestion above. Can't do that based on above.
On 2015/09/04 11:52:53, Henrik Grunell wrote: > jwd@: please review tools/metrics/histograms/histograms.xml > > Tommi: ptal. Oh, and Dale as well ptal.
The Peek() comment isn't accurate anymore, it was written before we had a posix impl: https://codereview.chromium.org/464020 You should be fine to use Peek as we've suggested.
https://codereview.chromium.org/1302423006/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1302423006/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:16654: + The number of input audio read verifications that missed its realtime Is it the number or the % as the units suggests?
On 2015/09/04 17:15:02, DaleCurtis wrote: > The Peek() comment isn't accurate anymore, it was written before we had a posix > impl: https://codereview.chromium.org/464020 > > You should be fine to use Peek as we've suggested. OK, cool. Now peeking and reading the amount available once.
thestig@: Updated comment in base/sync_socket.h, apparently obsolete information. Ptal. Tommi & Dale: ptal. (jwd@: ptal as requested earlier.) https://codereview.chromium.org/1302423006/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1302423006/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:16654: + The number of input audio read verifications that missed its realtime On 2015/09/04 19:20:34, Jesse Doherty wrote: > Is it the number or the % as the units suggests? %. Fixed.
lgtm
lgtm
Dale: I assume you want a final look on this CL?
lgtm https://codereview.chromium.org/1302423006/diff/160001/content/browser/render... File content/browser/renderer_host/media/audio_input_sync_writer.cc (right): https://codereview.chromium.org/1302423006/diff/160001/content/browser/render... content/browser/renderer_host/media/audio_input_sync_writer.cc:132: scoped_ptr<uint32_t[]> indices(new uint32_t[number_of_indices_available]); I'd allocate this once at full capacity and reuse it, but up to you.
The CQ bit was checked by grunell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, tommi@chromium.org, dalecurtis@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/1302423006/#ps180001 (title: "Fixed aligment of data buffer in unit test. Some other Win compile fixes. Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1302423006/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302423006/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by grunell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, tommi@chromium.org, dalecurtis@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/1302423006/#ps200001 (title: "Fixed log call expectations for Android in unit test. Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1302423006/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302423006/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by grunell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1302423006/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302423006/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/8d9071da52c70d300bfc0cdc0448c564b39764f4 Cr-Commit-Position: refs/heads/master@{#348156}
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/8d9071da52c70d300bfc0cdc0448c564b39764f4 Cr-Commit-Position: refs/heads/master@{#348156} |
