Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(20)

Issue 2004283002: AudioConverter: Express delay in frames rather than msec. (Closed)

Created:
4 years, 7 months ago by chcunningham
Modified:
4 years, 7 months ago
CC:
avayvod+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, imcheng+watch_chromium.org, isheriff+watch_chromium.org, jam, jasonroberts+watch_google.com, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

AudioConverter: Express delay in frames rather than msec. This is a todo from https://codereview.chromium.org/1687213002/ We already use frames on input and output sides of AudioConverter, so this unifies things and deletes some conversions. It also paves the way for another cleanup CL: https://codereview.chromium.org/2004963002/ BUG=587522 Committed: https://crrev.com/a3ff45e4f96a7876e69d2052f53586389783ab3f Cr-Commit-Position: refs/heads/master@{#396256}

Patch Set 1 #

Total comments: 15

Patch Set 2 : Feedback fixes #

Total comments: 4

Patch Set 3 : Missed files & removed rounding #

Unified diffs Side-by-side diffs Delta from patch set Stats (+106 lines, -132 lines) Patch
M chrome/browser/copresence/chrome_whispernet_client_browsertest.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/renderer/media/cast_rtp_stream.cc View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/speech/speech_recognizer_impl.cc View 2 chunks +3 lines, -2 lines 0 comments Download
M content/renderer/media/audio_track_recorder.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M content/renderer/media/speech_recognition_audio_sink.h View 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/media/speech_recognition_audio_sink.cc View 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/media/webrtc_local_audio_source_provider.h View 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/media/webrtc_local_audio_source_provider.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M extensions/renderer/api/display_source/wifi_display/wifi_display_audio_encoder_lpcm.cc View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M media/audio/audio_output_resampler.cc View 1 2 4 chunks +7 lines, -8 lines 0 comments Download
M media/audio/simple_sources.h View 1 chunk +1 line, -2 lines 0 comments Download
M media/audio/simple_sources.cc View 1 chunk +1 line, -1 line 0 comments Download
M media/audio/virtual_audio_input_stream.cc View 1 chunk +1 line, -1 line 0 comments Download
M media/audio/virtual_audio_output_stream.h View 1 chunk +1 line, -2 lines 0 comments Download
M media/audio/virtual_audio_output_stream.cc View 1 2 1 chunk +5 lines, -7 lines 0 comments Download
M media/base/audio_buffer_converter.h View 1 chunk +1 line, -2 lines 0 comments Download
M media/base/audio_buffer_converter.cc View 1 chunk +1 line, -1 line 0 comments Download
M media/base/audio_converter.h View 1 3 chunks +9 lines, -8 lines 0 comments Download
M media/base/audio_converter.cc View 1 7 chunks +22 lines, -25 lines 0 comments Download
M media/base/audio_converter_perftest.cc View 1 1 chunk +1 line, -2 lines 0 comments Download
M media/base/audio_converter_unittest.cc View 1 chunk +14 lines, -15 lines 0 comments Download
M media/base/audio_renderer_mixer.cc View 1 chunk +1 line, -7 lines 0 comments Download
M media/base/audio_renderer_mixer_input.h View 1 chunk +1 line, -2 lines 0 comments Download
M media/base/audio_renderer_mixer_input.cc View 1 chunk +1 line, -7 lines 0 comments Download
M media/base/audio_renderer_mixer_input_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M media/base/fake_audio_render_callback.h View 3 chunks +5 lines, -8 lines 0 comments Download
M media/base/fake_audio_render_callback.cc View 3 chunks +7 lines, -5 lines 0 comments Download
M media/base/fake_audio_renderer_sink.h View 1 chunk +2 lines, -4 lines 0 comments Download
M media/base/fake_audio_renderer_sink.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M media/base/loopback_audio_converter.h View 1 chunk +1 line, -2 lines 0 comments Download
M media/base/loopback_audio_converter.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M media/blink/webaudiosourceprovider_impl_unittest.cc View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M media/cast/test/fake_media_source.h View 1 chunk +2 lines, -2 lines 0 comments Download
M media/cast/test/fake_media_source.cc View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 24 (8 generated)
chcunningham
4 years, 7 months ago (2016-05-24 01:31:37 UTC) #2
chcunningham
https://codereview.chromium.org/2004283002/diff/1/media/base/audio_converter_unittest.cc File media/base/audio_converter_unittest.cc (left): https://codereview.chromium.org/2004283002/diff/1/media/base/audio_converter_unittest.cc#oldcode232 media/base/audio_converter_unittest.cc:232: EXPECT_EQ(expected_last_delay_milliseconds, This old method for building an expectation is ...
4 years, 7 months ago (2016-05-24 01:34:38 UTC) #3
DaleCurtis
lgtm, but +grunell who had some commentary last time. https://codereview.chromium.org/2004283002/diff/1/media/audio/audio_output_resampler.cc File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2004283002/diff/1/media/audio/audio_output_resampler.cc#newcode390 media/audio/audio_output_resampler.cc:390: ...
4 years, 7 months ago (2016-05-24 20:27:02 UTC) #5
Henrik Grunell
Took a quick look, mainly from webrtc perspective. I didn't do a thorough review of ...
4 years, 7 months ago (2016-05-25 14:48:38 UTC) #6
chcunningham
https://codereview.chromium.org/2004283002/diff/1/media/audio/audio_output_resampler.cc File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2004283002/diff/1/media/audio/audio_output_resampler.cc#newcode390 media/audio/audio_output_resampler.cc:390: // TODO(dalecurtis): Stop passing bytes around, it doesn't make ...
4 years, 7 months ago (2016-05-25 18:21:15 UTC) #7
chcunningham
+xiyuan@ for copresence +tommi@ for speech
4 years, 7 months ago (2016-05-25 18:25:42 UTC) #9
miu
On the whole, I wonder if it's a good idea to use |uint32_t| rather than ...
4 years, 7 months ago (2016-05-25 21:41:06 UTC) #11
chcunningham
+alexander.shalamov for wifi_display Thanks miu, > The reason you may want a signed int is ...
4 years, 7 months ago (2016-05-25 22:10:43 UTC) #13
shalamov
lgtm
4 years, 7 months ago (2016-05-26 07:16:10 UTC) #14
tommi (sloooow) - chröme
lgtm for speech
4 years, 7 months ago (2016-05-26 08:23:37 UTC) #15
xiyuan
copresence lgtm
4 years, 7 months ago (2016-05-26 16:13:01 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2004283002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2004283002/40001
4 years, 7 months ago (2016-05-26 17:36:50 UTC) #19
miu
On 2016/05/25 22:10:43, chcunningham wrote: > > The reason you may want a signed int ...
4 years, 7 months ago (2016-05-26 18:51:16 UTC) #20
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 7 months ago (2016-05-26 19:37:56 UTC) #21
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/a3ff45e4f96a7876e69d2052f53586389783ab3f Cr-Commit-Position: refs/heads/master@{#396256}
4 years, 7 months ago (2016-05-26 19:39:06 UTC) #23
DaleCurtis
4 years, 7 months ago (2016-05-26 21:15:07 UTC) #24
Message was sent while issue was closed.
On 2016/05/26 at 18:51:16, miu wrote:
> On 2016/05/25 22:10:43, chcunningham wrote:
> > > The reason you may want a signed int is because there could be use cases
> > (client
> > > code) where the frame delay could be negative: This would be interpreted
as "I
> > 
> > I'm confused. As currently used, we are always converting audio that will be
> > played out in the future. This audio also happens to have been captured in
the
> > past. IIUC, that is the only way for this to work? I can't convert audio
that
> > isn't captured yet, nor can I convert audio that will be played in the past.

> 
> I'm thinking of the remoting use cases, where Chrome is receiving audio ahead
of its scheduled playout time. For example, the code/comments here explain a
real-world case for negative delays:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/me...
> 
> Granted, we're not using AudioConverter there, but if there's a use for it
down the road, we'll have to do some hackery like offset the actual delay values
by +1<<31 before passing them into AudioConverter.
> 
> Oh, and I see that code I linked to above calls
AudioCapturerSource::Capture(), where you have a TODO comment to change from
milliseconds to frame delay:
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_c...
> 
> So, see you in the next CL?  ;-)

Hmm? Delay is always positive to indicate future play out. Negative delay would
mean you expect the data to be played out in the past. Is that what you're
trying to do?

Powered by Google App Engine
This is Rietveld 408576698