Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1308153005:
[Chromecast] Plumbs raw audio through CMA backend. (Closed)
|
Created:
5 years, 3 months ago by alokp Modified:
5 years, 3 months ago CC:
chromium-reviews, gunsch+watch_chromium.org, lcwu+watch_chromium.org, feature-media-reviews_chromium.org, DaleCurtis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Plumbs raw audio through CMA backend.
Raw audio path is used by webaudio and pepper audio. Plumbing it
through the CMA backend allows us to consolidate various audio
output paths with CMA backend, which in turn eliminates the need
for additional shlib interface to output raw audio.
Committed: https://crrev.com/49923cbbb37f491facae899f9470691d87f5de40
Cr-Commit-Position: refs/heads/master@{#349873}
Patch Set 1 #
Total comments: 27
Patch Set 2 : addressed comments #Patch Set 3 : adjusted logging #
Total comments: 5
Patch Set 4 : LOG(INFO) -> VLOG(1) #Patch Set 5 : rebase #Patch Set 6 : changed default sample rate #
Total comments: 23
Patch Set 7 : addressed comments #Patch Set 8 : added dcheck for audio thread #Patch Set 9 : added Cast prefix to class names #Patch Set 10 : added () after empty constructor #
Total comments: 8
Patch Set 11 : addressed Dale's suggestions #Patch Set 12 : fixed cast_shell compile error #Messages
Total messages: 49 (14 generated)
alokp@chromium.org changed reviewers: + halliwell@chromium.org, kmackay@chromium.org, slan@chromium.org
dalecurtis: FYI but I would appreciate any feedback. This is the patch I mentioned yesterday during our meeting. halliwell: owner and usage of shlib API kmackay: original reviewer slan: BUILD.gn
Biggest concern is whether this will actually work correctly with current backends. The use of the clock will map down to using hardware clock APIs intended for A/V sync, which the current Web Audio code path doesn't use. I think there may be unresolved bugs when creating multiple pipelines using clock components. So I wonder if this will work correctly when multiple streams are in play, and/or used together with a media element. So ... some thorough testing is warranted. And if any of that is a problem, perhaps we can find a way to support CMA backends without a clock. https://codereview.chromium.org/1308153005/diff/1/chromecast/browser/cast_con... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/1308153005/diff/1/chromecast/browser/cast_con... chromecast/browser/cast_content_browser_client.cc:78: LOG(INFO) << __FUNCTION__; remove https://codereview.chromium.org/1308153005/diff/1/chromecast/browser/cast_con... chromecast/browser/cast_content_browser_client.cc:86: } Question: why do we need this function now? Isn't the point of going through CMA to eliminate platform-specific AudioManagerFactory implementations? https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... File chromecast/media/audio/audio_manager.h (right): https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_manager.h:35: virtual scoped_ptr<MediaPipelineBackend> CreateMediaPipelineBackend(); I'm not clear what this is for. Can't we always just create the media pipeline backend through CastMediaShlib? https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... File chromecast/media/audio/audio_output_stream.cc (right): https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:39: } It seems unfortunate that we have to do this. The ultimate intent of the clock is to implement AV sync, which we don't need here. https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:80: LOG(INFO) << __FUNCTION__; do we really need all these info logs?
Looks really solid! https://codereview.chromium.org/1308153005/diff/1/chromecast/browser/cast_con... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/1308153005/diff/1/chromecast/browser/cast_con... chromecast/browser/cast_content_browser_client.cc:86: } On 2015/08/28 15:14:18, halliwell wrote: > Question: why do we need this function now? Isn't the point of going through > CMA to eliminate platform-specific AudioManagerFactory implementations? Right. In fact, this probably doesn't even need to be passed in to CastBrowserMainParts anymore. It should just call SetFactory(chromecast::AudioManagerFactory()), unless on Android. https://codereview.chromium.org/1308153005/diff/1/chromecast/media/BUILD.gn File chromecast/media/BUILD.gn (right): https://codereview.chromium.org/1308153005/diff/1/chromecast/media/BUILD.gn#n... chromecast/media/BUILD.gn:18: "cma/backend/audio_video_pipeline_device_unittest.cc", Add unittests here? https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... File chromecast/media/audio/audio_manager.h (right): https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_manager.h:10: namespace chromecast { nit: This can simply be declared inside the namespace below, I think. Is there preference in Chrome? https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... File chromecast/media/audio/audio_output_stream.cc (right): https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:129: audio_worker_.reset(); This class relies on Start(), Stop() , and dtor all running on same thread. Did you consider putting checks in place to ensure that the caller of AudioOutputStream is observing this model? (i.e. calling Start(), Open(), Stop(), and Close() on the same thread?) https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:187: if (audio_device_busy_) { In what situation does this occur? audio_worker_ will delay a full buffer before PushFrame is called again. Could this result in underrun? https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:207: base::Bind(&AudioOutputStream::OnPushFrameStatus, Is there somewhere in the documentation that makes it clear that this callback must be run on the same thread as PushFrame()? I didn't see it on a quick glance through the header. https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:226: DCHECK(audio_device_busy_); Is it possible that Stop() is called, while this callback is pending, causing DCHECK when it runs?
https://codereview.chromium.org/1308153005/diff/1/chromecast/browser/cast_con... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/1308153005/diff/1/chromecast/browser/cast_con... chromecast/browser/cast_content_browser_client.cc:78: LOG(INFO) << __FUNCTION__; On 2015/08/28 15:14:18, halliwell wrote: > remove Done. https://codereview.chromium.org/1308153005/diff/1/chromecast/browser/cast_con... chromecast/browser/cast_content_browser_client.cc:86: } On 2015/08/31 16:59:56, slan wrote: > On 2015/08/28 15:14:18, halliwell wrote: > > Question: why do we need this function now? Isn't the point of going through > > CMA to eliminate platform-specific AudioManagerFactory implementations? > > Right. In fact, this probably doesn't even need to be passed in to > CastBrowserMainParts anymore. It should just call > SetFactory(chromecast::AudioManagerFactory()), unless on Android. Long story - I will explain this offline. https://codereview.chromium.org/1308153005/diff/1/chromecast/media/BUILD.gn File chromecast/media/BUILD.gn (right): https://codereview.chromium.org/1308153005/diff/1/chromecast/media/BUILD.gn#n... chromecast/media/BUILD.gn:18: "cma/backend/audio_video_pipeline_device_unittest.cc", On 2015/08/31 16:59:56, slan wrote: > Add unittests here? Aren't we trying to follow the test_support approach? https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... File chromecast/media/audio/audio_manager.h (right): https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_manager.h:10: namespace chromecast { On 2015/08/31 16:59:56, slan wrote: > nit: This can simply be declared inside the namespace below, I think. Is there > preference in Chrome? Done. https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_manager.h:35: virtual scoped_ptr<MediaPipelineBackend> CreateMediaPipelineBackend(); On 2015/08/28 15:14:18, halliwell wrote: > I'm not clear what this is for. Can't we always just create the media pipeline > backend through CastMediaShlib? Will explain offline. https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... File chromecast/media/audio/audio_output_stream.cc (right): https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:39: } On 2015/08/28 15:14:18, halliwell wrote: > It seems unfortunate that we have to do this. The ultimate intent of the clock > is to implement AV sync, which we don't need here. Right. Although removing the requirement of a clock seems out of scope for this patch. If our backend implementations can support as many clocks as number of audio streams, this does not seem too bad. But we do need to verify that multiple clocks is supported. https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:80: LOG(INFO) << __FUNCTION__; On 2015/08/28 15:14:18, halliwell wrote: > do we really need all these info logs? We did have this in one backend implementation and I think it is a useful one. These do not fire frequently - just once for each output stream. https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:129: audio_worker_.reset(); On 2015/08/31 16:59:56, slan wrote: > This class relies on Start(), Stop() , and dtor all running on same thread. Did > you consider putting checks in place to ensure that the caller of > AudioOutputStream is observing this model? (i.e. calling Start(), Open(), > Stop(), and Close() on the same thread?) AudioManager already has this CHECK. In addition FakeAudioWorker does bunch of checks. https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:187: if (audio_device_busy_) { On 2015/08/31 16:59:56, slan wrote: > In what situation does this occur? audio_worker_ will delay a full buffer before > PushFrame is called again. Could this result in underrun? This happens when the backend buffer is full - opposite of underrun. We do not lose any frames though. We just avoid pulling more data. https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:207: base::Bind(&AudioOutputStream::OnPushFrameStatus, On 2015/08/31 16:59:56, slan wrote: > Is there somewhere in the documentation that makes it clear that this callback > must be run on the same thread as PushFrame()? I didn't see it on a quick glance > through the header. This guarantee should be provided by AudioPipelineDevice. In general, we shoudl assume that the callbacks run on the same thread unless indicated otherwise. https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:226: DCHECK(audio_device_busy_); On 2015/08/31 16:59:56, slan wrote: > Is it possible that Stop() is called, while this callback is pending, causing > DCHECK when it runs? You are right. Luke: Do we ignore the callback if AudioPipelineDevice status is changed to Idle?
https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... File chromecast/media/audio/audio_output_stream.cc (right): https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:80: LOG(INFO) << __FUNCTION__; On 2015/09/01 00:23:12, Alok Priyadarshi wrote: > On 2015/08/28 15:14:18, halliwell wrote: > > do we really need all these info logs? > > We did have this in one backend implementation and I think it is a useful one. > These do not fire frequently - just once for each output stream. Frequency seems fine, but it doesn't seem like much thought has gone into them or what type of problems you could track down with them. For instance: * This one might as well be moved down to the success case at the end, since we already have LOG_WARNING for all the failure cases * If you output 'this' then you could correlate the constructor (with the params), Open, Close etc with one another when there multiple streams in play. These are just examples, I'm not so familiar with this code or what info is going to be most useful here. I agree that logging is useful - it's often the main thing to go off when triaging bugs - but all the more reason to think carefully about what information to log. These feel a bit like the kind of thing you insert when developing a feature just to check your code is getting called as you expect ... and may not be so useful when it comes to figuring out tricky bugs later on. I could be missing the usefulness of these though, let me know what you think :) https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:207: base::Bind(&AudioOutputStream::OnPushFrameStatus, On 2015/09/01 00:23:11, Alok Priyadarshi wrote: > On 2015/08/31 16:59:56, slan wrote: > > Is there somewhere in the documentation that makes it clear that this callback > > must be run on the same thread as PushFrame()? I didn't see it on a quick > glance > > through the header. > > This guarantee should be provided by AudioPipelineDevice. In general, we shoudl > assume that the callbacks run on the same thread unless indicated otherwise. AudioPipelineDevice is a platform backend, we didn't actually document this requirement yet, and we don't have any compliance tests. But we do have a DCHECK in the regular cma pipeline, so would be surprised if any backends are violating this. I think it's reasonable to assume here. A DCHECK could be worthwhile for now if it's easy enough to do. https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:226: DCHECK(audio_device_busy_); On 2015/09/01 00:23:11, Alok Priyadarshi wrote: > On 2015/08/31 16:59:56, slan wrote: > > Is it possible that Stop() is called, while this callback is pending, causing > > DCHECK when it runs? > > You are right. > > Luke: Do we ignore the callback if AudioPipelineDevice status is changed to > Idle? Comment on PushFrame says "Pushing a pending frame should be aborted if the state returns to kStateIdle, and |completion_cb| need not be invoked.". I strongly suspect existing backends do not invoke completion_cb in this case. Again, we could possibly tighten the wording to say "must not be invoked" and we could add to compliance test. I think for now, what you have should be ok.
https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... File chromecast/media/audio/audio_output_stream.cc (right): https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... chromecast/media/audio/audio_output_stream.cc:80: LOG(INFO) << __FUNCTION__; On 2015/09/01 01:10:32, halliwell wrote: > On 2015/09/01 00:23:12, Alok Priyadarshi wrote: > > On 2015/08/28 15:14:18, halliwell wrote: > > > do we really need all these info logs? > > > > We did have this in one backend implementation and I think it is a useful one. > > These do not fire frequently - just once for each output stream. > > Frequency seems fine, but it doesn't seem like much thought has gone into them > or what type of problems you could track down with them. For instance: > * This one might as well be moved down to the success case at the end, since we > already have LOG_WARNING for all the failure cases > * If you output 'this' then you could correlate the constructor (with the > params), Open, Close etc with one another when there multiple streams in play. > > These are just examples, I'm not so familiar with this code or what info is > going to be most useful here. I agree that logging is useful - it's often the > main thing to go off when triaging bugs - but all the more reason to think > carefully about what information to log. These feel a bit like the kind of > thing you insert when developing a feature just to check your code is getting > called as you expect ... and may not be so useful when it comes to figuring out > tricky bugs later on. > > I could be missing the usefulness of these though, let me know what you think :) Good point about adding 'this' to the logs. I have done that and also moved them around as you suggested. I am not sure which logging function we use to log these kind of messages in chromecast. In chrome I would probably use DVLOG. Would that be OK?
On 2015/09/01 17:34:44, Alok Priyadarshi wrote: > https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... > File chromecast/media/audio/audio_output_stream.cc (right): > > https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... > chromecast/media/audio/audio_output_stream.cc:80: LOG(INFO) << __FUNCTION__; > On 2015/09/01 01:10:32, halliwell wrote: > > On 2015/09/01 00:23:12, Alok Priyadarshi wrote: > > > On 2015/08/28 15:14:18, halliwell wrote: > > > > do we really need all these info logs? > > > > > > We did have this in one backend implementation and I think it is a useful > one. > > > These do not fire frequently - just once for each output stream. > > > > Frequency seems fine, but it doesn't seem like much thought has gone into them > > or what type of problems you could track down with them. For instance: > > * This one might as well be moved down to the success case at the end, since > we > > already have LOG_WARNING for all the failure cases > > * If you output 'this' then you could correlate the constructor (with the > > params), Open, Close etc with one another when there multiple streams in play. > > > > These are just examples, I'm not so familiar with this code or what info is > > going to be most useful here. I agree that logging is useful - it's often the > > main thing to go off when triaging bugs - but all the more reason to think > > carefully about what information to log. These feel a bit like the kind of > > thing you insert when developing a feature just to check your code is getting > > called as you expect ... and may not be so useful when it comes to figuring > out > > tricky bugs later on. > > > > I could be missing the usefulness of these though, let me know what you think > :) > > Good point about adding 'this' to the logs. I have done that and also moved them > around as you suggested. I am not sure which logging function we use to log > these kind of messages in chromecast. In chrome I would probably use DVLOG. > Would that be OK? Isn't DVLOG stripped out for releases? Then it comes down to whether you think the logging is important for analysing bug reports from production, or whether it's just useful for while you're developing :) Personally, I'm more likely to use LOG for that reason ...
On 2015/09/01 21:13:33, halliwell wrote: > On 2015/09/01 17:34:44, Alok Priyadarshi wrote: > > > https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... > > File chromecast/media/audio/audio_output_stream.cc (right): > > > > > https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... > > chromecast/media/audio/audio_output_stream.cc:80: LOG(INFO) << __FUNCTION__; > > On 2015/09/01 01:10:32, halliwell wrote: > > > On 2015/09/01 00:23:12, Alok Priyadarshi wrote: > > > > On 2015/08/28 15:14:18, halliwell wrote: > > > > > do we really need all these info logs? > > > > > > > > We did have this in one backend implementation and I think it is a useful > > one. > > > > These do not fire frequently - just once for each output stream. > > > > > > Frequency seems fine, but it doesn't seem like much thought has gone into > them > > > or what type of problems you could track down with them. For instance: > > > * This one might as well be moved down to the success case at the end, since > > we > > > already have LOG_WARNING for all the failure cases > > > * If you output 'this' then you could correlate the constructor (with the > > > params), Open, Close etc with one another when there multiple streams in > play. > > > > > > These are just examples, I'm not so familiar with this code or what info is > > > going to be most useful here. I agree that logging is useful - it's often > the > > > main thing to go off when triaging bugs - but all the more reason to think > > > carefully about what information to log. These feel a bit like the kind of > > > thing you insert when developing a feature just to check your code is > getting > > > called as you expect ... and may not be so useful when it comes to figuring > > out > > > tricky bugs later on. > > > > > > I could be missing the usefulness of these though, let me know what you > think > > :) > > > > Good point about adding 'this' to the logs. I have done that and also moved > them > > around as you suggested. I am not sure which logging function we use to log > > these kind of messages in chromecast. In chrome I would probably use DVLOG. > > Would that be OK? > > Isn't DVLOG stripped out for releases? Then it comes down to whether you think > the logging is important for analysing bug reports from production, or whether > it's just useful for while you're developing :) Personally, I'm more likely to > use LOG for that reason ... Sorry I meant VLOG. I remember that we capture upto VLOG(2)? I am not sure which one is preferred in chromecast LOG or VLOG.
https://codereview.chromium.org/1308153005/diff/40001/chromecast/media/audio/... File chromecast/media/audio/audio_output_stream.cc (right): https://codereview.chromium.org/1308153005/diff/40001/chromecast/media/audio/... chromecast/media/audio/audio_output_stream.cc:74: LOG(INFO) << "Audio Parameters: " << audio_params_.AsHumanReadableString(); nit: can we put these back on the same log line? https://codereview.chromium.org/1308153005/diff/40001/chromecast/media/audio/... chromecast/media/audio/audio_output_stream.cc:190: << " skipped because audio device is busy."; hmmm ... isn't this log going to happen all the time? I don't know how the feeding of audio data happens with AudioOutputStream, but with media pipelines, we essentially always hit this condition where the hardware buffer is full and have to wait to push more frames. We wouldn't want to log for every frame in that case.
On 2015/09/01 21:25:06, Alok Priyadarshi wrote: > On 2015/09/01 21:13:33, halliwell wrote: > > On 2015/09/01 17:34:44, Alok Priyadarshi wrote: > > > > > > https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... > > > File chromecast/media/audio/audio_output_stream.cc (right): > > > > > > > > > https://codereview.chromium.org/1308153005/diff/1/chromecast/media/audio/audi... > > > chromecast/media/audio/audio_output_stream.cc:80: LOG(INFO) << __FUNCTION__; > > > On 2015/09/01 01:10:32, halliwell wrote: > > > > On 2015/09/01 00:23:12, Alok Priyadarshi wrote: > > > > > On 2015/08/28 15:14:18, halliwell wrote: > > > > > > do we really need all these info logs? > > > > > > > > > > We did have this in one backend implementation and I think it is a > useful > > > one. > > > > > These do not fire frequently - just once for each output stream. > > > > > > > > Frequency seems fine, but it doesn't seem like much thought has gone into > > them > > > > or what type of problems you could track down with them. For instance: > > > > * This one might as well be moved down to the success case at the end, > since > > > we > > > > already have LOG_WARNING for all the failure cases > > > > * If you output 'this' then you could correlate the constructor (with the > > > > params), Open, Close etc with one another when there multiple streams in > > play. > > > > > > > > These are just examples, I'm not so familiar with this code or what info > is > > > > going to be most useful here. I agree that logging is useful - it's often > > the > > > > main thing to go off when triaging bugs - but all the more reason to think > > > > carefully about what information to log. These feel a bit like the kind > of > > > > thing you insert when developing a feature just to check your code is > > getting > > > > called as you expect ... and may not be so useful when it comes to > figuring > > > out > > > > tricky bugs later on. > > > > > > > > I could be missing the usefulness of these though, let me know what you > > think > > > :) > > > > > > Good point about adding 'this' to the logs. I have done that and also moved > > them > > > around as you suggested. I am not sure which logging function we use to log > > > these kind of messages in chromecast. In chrome I would probably use DVLOG. > > > Would that be OK? > > > > Isn't DVLOG stripped out for releases? Then it comes down to whether you > think > > the logging is important for analysing bug reports from production, or whether > > it's just useful for while you're developing :) Personally, I'm more likely > to > > use LOG for that reason ... > > Sorry I meant VLOG. I remember that we capture upto VLOG(2)? I am not sure which > one is preferred in chromecast LOG or VLOG. Yes, I think VLOG is preferred because it can be controlled in a more fine-grained manner. I see plenty of VLOG usage in chromecast/.
https://codereview.chromium.org/1308153005/diff/40001/chromecast/media/audio/... File chromecast/media/audio/audio_manager.cc (right): https://codereview.chromium.org/1308153005/diff/40001/chromecast/media/audio/... chromecast/media/audio/audio_manager.cc:52: audio_task_runner_.reset(new TaskRunnerImpl()); We might want to DCHECK that CreateMediaPipelineBackend() is always called on the same thread.
https://codereview.chromium.org/1308153005/diff/40001/chromecast/media/audio/... File chromecast/media/audio/audio_output_stream.cc (right): https://codereview.chromium.org/1308153005/diff/40001/chromecast/media/audio/... chromecast/media/audio/audio_output_stream.cc:190: << " skipped because audio device is busy."; On 2015/09/01 21:27:03, halliwell wrote: > hmmm ... isn't this log going to happen all the time? > > I don't know how the feeding of audio data happens with AudioOutputStream, but > with media pipelines, we essentially always hit this condition where the > hardware buffer is full and have to wait to push more frames. We wouldn't want > to log for every frame in that case. No it should not fire all the time. AudioWorker calls PushFrame at playback rate, so the device being busy is an error condition.
https://codereview.chromium.org/1308153005/diff/40001/chromecast/media/audio/... File chromecast/media/audio/audio_manager.cc (right): https://codereview.chromium.org/1308153005/diff/40001/chromecast/media/audio/... chromecast/media/audio/audio_manager.cc:52: audio_task_runner_.reset(new TaskRunnerImpl()); On 2015/09/01 23:04:47, kmackay wrote: > We might want to DCHECK that CreateMediaPipelineBackend() is always called on > the same thread. In production it should always be called on the AudioThread. But I do not have an AudioThread in unittests, so I am not sure how to enforce it. I will see if I can fake an AudioThread in unittests.
alokp@chromium.org changed reviewers: + gunsch@chromium.org, lcwu@google.com
halliwell/lcwu/gunsch: Need OWNER stamp
LGTM
mostly nits, but a few critical questions https://codereview.chromium.org/1308153005/diff/100001/chromecast/browser/cas... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/browser/cas... chromecast/browser/cast_content_browser_client.cc:90: return make_scoped_ptr(new media::AudioManagerFactory); From my understanding of this CL, this removes the need for CastContentBrowserClientInternal::CreateAudioManagerFactory. Is that correct? If so, we should remove this API entirely and just have CastBrowserMainParts create a media::AudioManagerFactory directly instead of passing it through from here. https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... File chromecast/media/audio/audio_manager.h (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_manager.h:18: class AudioManager : public ::media::AudioManagerBase { I know it's namespaced, but can we call this CastAudioManager to better distinguish from ::media::AudioManager? This is already a convention with our other implementation classes (CastBrowserCdmFactory, CastBrowserContext, CastMetricsServiceClient, etc. https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_manager.h:20: AudioManager(::media::AudioLogFactory* audio_log_factory); nit: explicit https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_manager.h:32: // In production, this must be called on audio thread. Why "in production"? Shouldn't this assumption either always or never be the case? https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_manager.h:55: DISALLOW_COPY_AND_ASSIGN(AudioManager); nit: #include "base/macros.h" https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... File chromecast/media/audio/audio_manager_factory.h (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_manager_factory.h:14: class AudioManagerFactory : public ::media::AudioManagerFactory { same naming comment: CastAudioManagerFactory https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... File chromecast/media/audio/audio_output_stream.h (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_output_stream.h:54: DISALLOW_COPY_AND_ASSIGN(AudioOutputStream); nit: #include "base/macros.h" https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... File chromecast/media/audio/audio_output_stream_unittest.cc (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_output_stream_unittest.cc:17: const char kDefaultDeviceId[] = ""; can this be inside the another anonymous namespace, or vice versa? no need for two https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/media... File chromecast/media/media.gyp (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/media... chromecast/media/media.gyp:26: 'target_name': 'media_audio', I don't see any GYP targets that depend on this?
Thanks for the quick review. https://codereview.chromium.org/1308153005/diff/100001/chromecast/browser/cas... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/browser/cas... chromecast/browser/cast_content_browser_client.cc:90: return make_scoped_ptr(new media::AudioManagerFactory); On 2015/09/18 21:23:07, gunsch wrote: > From my understanding of this CL, this removes the need for > CastContentBrowserClientInternal::CreateAudioManagerFactory. Is that correct? > > If so, we should remove this API entirely and just have CastBrowserMainParts > create a media::AudioManagerFactory directly instead of passing it through from > here. The internal implementation needs to create a different AudioManager/AudioManagerFactory. https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... File chromecast/media/audio/audio_manager.h (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_manager.h:18: class AudioManager : public ::media::AudioManagerBase { On 2015/09/18 21:23:07, gunsch wrote: > I know it's namespaced, but can we call this CastAudioManager to better > distinguish from ::media::AudioManager? This is already a convention with our > other implementation classes (CastBrowserCdmFactory, CastBrowserContext, > CastMetricsServiceClient, etc. SGTM. Should I rename AudioOutputStream to CastAudioOutputStream too? https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_manager.h:20: AudioManager(::media::AudioLogFactory* audio_log_factory); On 2015/09/18 21:23:07, gunsch wrote: > nit: explicit Done. https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_manager.h:32: // In production, this must be called on audio thread. On 2015/09/18 21:23:07, gunsch wrote: > Why "in production"? Shouldn't this assumption either always or never be the > case? This is not true in unittest, where I do not create an audio thread. I will check if I can fake the audio thread in the unittest. https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_manager.h:55: DISALLOW_COPY_AND_ASSIGN(AudioManager); On 2015/09/18 21:23:07, gunsch wrote: > nit: #include "base/macros.h" Done. https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... File chromecast/media/audio/audio_manager_factory.h (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_manager_factory.h:14: class AudioManagerFactory : public ::media::AudioManagerFactory { On 2015/09/18 21:23:07, gunsch wrote: > same naming comment: CastAudioManagerFactory Acknowledged. https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... File chromecast/media/audio/audio_output_stream.h (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_output_stream.h:23: class AudioOutputStream : public ::media::AudioOutputStream { Should we rename this to CastAudioOutputStream too? https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_output_stream.h:54: DISALLOW_COPY_AND_ASSIGN(AudioOutputStream); On 2015/09/18 21:23:07, gunsch wrote: > nit: #include "base/macros.h" Done. https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... File chromecast/media/audio/audio_output_stream_unittest.cc (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_output_stream_unittest.cc:17: const char kDefaultDeviceId[] = ""; On 2015/09/18 21:23:07, gunsch wrote: > can this be inside the another anonymous namespace, or vice versa? no need for > two Done. https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/media... File chromecast/media/media.gyp (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/media... chromecast/media/media.gyp:26: 'target_name': 'media_audio', On 2015/09/18 21:23:07, gunsch wrote: > I don't see any GYP targets that depend on this? cast_media depends on media_audio.
https://codereview.chromium.org/1308153005/diff/100001/chromecast/browser/cas... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/browser/cas... chromecast/browser/cast_content_browser_client.cc:90: return make_scoped_ptr(new media::AudioManagerFactory); On 2015/09/18 21:42:14, Alok Priyadarshi wrote: > On 2015/09/18 21:23:07, gunsch wrote: > > From my understanding of this CL, this removes the need for > > CastContentBrowserClientInternal::CreateAudioManagerFactory. Is that correct? > > > > If so, we should remove this API entirely and just have CastBrowserMainParts > > create a media::AudioManagerFactory directly instead of passing it through > from > > here. > > The internal implementation needs to create a different > AudioManager/AudioManagerFactory. SGTM. Nit: prefer '()' when invoking empty constructors (not in style guide but has been mentioned by several committers and we try to follow it) https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... File chromecast/media/audio/audio_manager.h (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/audio... chromecast/media/audio/audio_manager.h:18: class AudioManager : public ::media::AudioManagerBase { On 2015/09/18 21:42:14, Alok Priyadarshi wrote: > On 2015/09/18 21:23:07, gunsch wrote: > > I know it's namespaced, but can we call this CastAudioManager to better > > distinguish from ::media::AudioManager? This is already a convention with our > > other implementation classes (CastBrowserCdmFactory, CastBrowserContext, > > CastMetricsServiceClient, etc. > > SGTM. Should I rename AudioOutputStream to CastAudioOutputStream too? Please do, thanks. https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/media... File chromecast/media/media.gyp (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/media/media... chromecast/media/media.gyp:26: 'target_name': 'media_audio', On 2015/09/18 21:42:15, Alok Priyadarshi wrote: > On 2015/09/18 21:23:07, gunsch wrote: > > I don't see any GYP targets that depend on this? > > cast_media depends on media_audio. Thanks, guess I just glossed over it.
https://codereview.chromium.org/1308153005/diff/100001/chromecast/browser/cas... File chromecast/browser/cast_content_browser_client.cc (right): https://codereview.chromium.org/1308153005/diff/100001/chromecast/browser/cas... chromecast/browser/cast_content_browser_client.cc:90: return make_scoped_ptr(new media::AudioManagerFactory); On 2015/09/18 21:57:06, gunsch wrote: > On 2015/09/18 21:42:14, Alok Priyadarshi wrote: > > On 2015/09/18 21:23:07, gunsch wrote: > > > From my understanding of this CL, this removes the need for > > > CastContentBrowserClientInternal::CreateAudioManagerFactory. Is that > correct? > > > > > > If so, we should remove this API entirely and just have CastBrowserMainParts > > > create a media::AudioManagerFactory directly instead of passing it through > > from > > > here. > > > > The internal implementation needs to create a different > > AudioManager/AudioManagerFactory. > > SGTM. > > Nit: prefer '()' when invoking empty constructors > > (not in style guide but has been mentioned by several committers and we try to > follow it) Done.
lgtm, thanks for the changes
note: I changed commit message to start with "[Chromecast]"
The CQ bit was checked by alokp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmackay@chromium.org Link to the patchset: https://codereview.chromium.org/1308153005/#ps180001 (title: "added () after empty constructor")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308153005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308153005/180001
alokp@chromium.org changed reviewers: + tommi@chromium.org
tommi: OWNER for media/audio dependency
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
alokp@chromium.org changed reviewers: + dalecurtis@chromium.org
Tommi/Dale: chromecast/media/DEPS
lgtm https://codereview.chromium.org/1308153005/diff/180001/chromecast/media/audio... File chromecast/media/audio/cast_audio_manager.cc (right): https://codereview.chromium.org/1308153005/diff/180001/chromecast/media/audio... chromecast/media/audio/cast_audio_manager.cc:14: const int kDefaultSampleRate = 48000; You'll probably want to tailor this for your whatever your silicon is using to avoid any extra resampling. https://codereview.chromium.org/1308153005/diff/180001/chromecast/media/audio... chromecast/media/audio/cast_audio_manager.cc:18: static const int kMinimumOutputBufferSize = 512; Up to you if you need these; again whatever the silicon is expecting. I think 512 might be too low for an alsa based product though. https://codereview.chromium.org/1308153005/diff/180001/chromecast/media/audio... File chromecast/media/audio/cast_audio_output_stream.cc (right): https://codereview.chromium.org/1308153005/diff/180001/chromecast/media/audio... chromecast/media/audio/cast_audio_output_stream.cc:81: if ((format != ::media::AudioParameters::AUDIO_PCM_LINEAR) && Should be able to dcheck this, it should never happen. https://codereview.chromium.org/1308153005/diff/180001/chromecast/media/audio... chromecast/media/audio/cast_audio_output_stream.cc:206: new CastDecoderBufferImpl(new DecoderBufferAdapter(decoder_buffer)), Lots of new objects created for a high frequency callback, but maybe okay since they're all relatively small. You might consider a pool of DecoderBuffer if profiling shows allocation time dominating this function. See VideoFramePool for a case where we do this on video.
> https://codereview.chromium.org/1308153005/diff/180001/chromecast/media/audio... > chromecast/media/audio/cast_audio_output_stream.cc:206: new > CastDecoderBufferImpl(new DecoderBufferAdapter(decoder_buffer)), > Lots of new objects created for a high frequency callback, but maybe okay since > they're all relatively small. You might consider a pool of DecoderBuffer if > profiling shows allocation time dominating this function. See VideoFramePool for > a case where we do this on video. These allocations will go away once the upcoming CMA backend API changes are completed.
https://codereview.chromium.org/1308153005/diff/180001/chromecast/media/audio... File chromecast/media/audio/cast_audio_manager.cc (right): https://codereview.chromium.org/1308153005/diff/180001/chromecast/media/audio... chromecast/media/audio/cast_audio_manager.cc:14: const int kDefaultSampleRate = 48000; On 2015/09/18 22:42:36, DaleCurtis wrote: > You'll probably want to tailor this for your whatever your silicon is using to > avoid any extra resampling. Added TODO to query from media backend. https://codereview.chromium.org/1308153005/diff/180001/chromecast/media/audio... chromecast/media/audio/cast_audio_manager.cc:18: static const int kMinimumOutputBufferSize = 512; On 2015/09/18 22:42:36, DaleCurtis wrote: > Up to you if you need these; again whatever the silicon is expecting. I think > 512 might be too low for an alsa based product though. Ditto. Thanks for your suggestion. https://codereview.chromium.org/1308153005/diff/180001/chromecast/media/audio... File chromecast/media/audio/cast_audio_output_stream.cc (right): https://codereview.chromium.org/1308153005/diff/180001/chromecast/media/audio... chromecast/media/audio/cast_audio_output_stream.cc:81: if ((format != ::media::AudioParameters::AUDIO_PCM_LINEAR) && On 2015/09/18 22:42:36, DaleCurtis wrote: > Should be able to dcheck this, it should never happen. Done. https://codereview.chromium.org/1308153005/diff/180001/chromecast/media/audio... chromecast/media/audio/cast_audio_output_stream.cc:206: new CastDecoderBufferImpl(new DecoderBufferAdapter(decoder_buffer)), On 2015/09/18 22:42:36, DaleCurtis wrote: > Lots of new objects created for a high frequency callback, but maybe okay since > they're all relatively small. You might consider a pool of DecoderBuffer if > profiling shows allocation time dominating this function. See VideoFramePool for > a case where we do this on video. As Ken mentioned, we will fix this shortly.
The CQ bit was checked by alokp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org, dalecurtis@chromium.org, kmackay@chromium.org Link to the patchset: https://codereview.chromium.org/1308153005/#ps200001 (title: "addressed Dale's suggestions")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308153005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308153005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by tommi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308153005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308153005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by alokp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org, dalecurtis@chromium.org, kmackay@chromium.org Link to the patchset: https://codereview.chromium.org/1308153005/#ps220001 (title: "fixed cast_shell compile error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1308153005/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1308153005/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/49923cbbb37f491facae899f9470691d87f5de40 Cr-Commit-Position: refs/heads/master@{#349873} |
