Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2466463005:
Support (E)AC3 passthrough
|
Created:
4 years, 1 month ago by AndyWu Modified:
3 years, 4 months ago Reviewers:
tommi (sloooow) - chröme, Simeon, Raymond Toy, chcunningham, Mike West, DaleCurtis, halliwell, Tima Vaisburd CC:
Aaron Boodman, abarth-chromium, agrieve+watch_chromium.org, alokp+watch_chromium.org, avayvod+watch_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, feature-media-reviews_chromium.org, halliwell+watch_chromium.org, jam, lcwu+watch_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-media_chromium.org, posciak+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, xjz+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport (E)AC3 passthrough
When the connected HDMI receiver supports (E)AC3 passthrough, we
can directly pass raw compressed (E)AC3 bitstream to AudioTrack.
BUG=613385
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Patch Set 1 #Patch Set 2 : Improve CastMediaClient::IsSupportedPassthroughAudio() #
Total comments: 47
Patch Set 3 : Support (E)AC3 passthrough #Patch Set 4 : Revert changes in audio_renderer_mixer. #
Total comments: 3
Patch Set 5 : Change kSampleFormatRaw and kCodecRaw #Patch Set 6 : Clamp current media time to the last reported value #Patch Set 7 : Rebase the CL #Patch Set 8 : Use the terminology "compressed" instead of "raw" #Patch Set 9 : Drop AudioDecoderConfig changes #Patch Set 10 : Parsing (E)AC3 syncframes #Patch Set 11 : Rebase #Patch Set 12 : Rename is_compressed_format to is_bitstream_format #Patch Set 13 : Rebase #Patch Set 14 : Rebase #Patch Set 15 : Rebase and Refactor #Patch Set 16 : Add unit tests #
Total comments: 96
Messages
Total messages: 51 (12 generated)
Description was changed from ========== Support (E)AC3 passthrough When the connected HDMI receiver supports (E)AC3 passthrough, we can directly pass raw compressed (E)AC3 bitstream to AudioTrack. BUG=613385 ========== to ========== Support (E)AC3 passthrough When the connected HDMI receiver supports (E)AC3 passthrough, we can directly pass raw compressed (E)AC3 bitstream to AudioTrack. BUG=613385 ==========
tsunghung@chromium.org changed reviewers: + dalecurtis@chromium.org, halliwell@chromium.org, sanfin@chromium.org, timav@chromium.org
tsunghung@chromium.org changed reviewers: + tommi@chromium.org
Thanks for putting this together. Do you have a design doc for it? What are you planning to do about WebAudio and playbackRate changes?
I don't really have a design doc, since it's just a straight forward change: passing compressed bitstream from audio decoder all the way down to audio output stream. I guess I can write something to help reviewers. I have no idea about WebAudio, will consult with someone and get back to you. I don't see any proper way to support playback rate change. Do we have to support it? On 2016/10/31 22:23:43, DaleCurtis wrote: > Thanks for putting this together. Do you have a design doc for it? What are you > planning to do about WebAudio and playbackRate changes?
For Chromecast that's up to you if you want to have support or not. But you should probably make sure it doesn't break anything at least. If we were to enable this for Chrome proper we'd need to support fallback into software decoded audio.
dalecurtis@chromium.org changed reviewers: + chcunningham@chromium.org, rtoy@chromium.org
This is pretty far from a straight-forward change :) You're piercing all sorts of layers and cramming the concept of raw audio into them. You should document your design decisions either in the CL description or in a separate doc. You'll also need to modify WebAudioSourceProviderImpl() to receive silence when connected. +rtoy for WebAudio interaction concerns. Here is my initial round of comments. +chcunningham to assist in review. https://codereview.chromium.org/2466463005/diff/20001/chromecast/common/media... File chromecast/common/media/cast_media_client.h (right): https://codereview.chromium.org/2466463005/diff/20001/chromecast/common/media... chromecast/common/media/cast_media_client.h:35: bool IsSupportedPassthroughAudio(::media::AudioCodec codec) override; IsPassthroughAudioSupported() ? https://codereview.chromium.org/2466463005/diff/20001/media/audio/audio_outpu... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/audio/audio_outpu... media/audio/audio_output_device.cc:27: static const int kMaxFramesPerCompressedAudioBuffer = 4096; This needs to be a property of the AudioParameters class; that said where did you get this number from? https://codereview.chromium.org/2466463005/diff/20001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:51: private WorkerThread mWorkerThread; What's the point of this? Using a thread instead of callbacks is going to lead to being out of sync with playback and is not as efficient as using callbacks. https://chromiumcodereview.appspot.com/1525033003/ This one piece is complicated enough to separate into its own code review with unittests. We should also consider the question of whether it's worth it to continue using OpenSLES or if we should always use AudioTrack instead. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer... media/base/audio_buffer.cc:58: base::AlignedAlloc(data_size_, kChannelAlignment))); AlignedAlloc shouldn't be necessary in this case. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer... media/base/audio_buffer.cc:59: channel_data_.reserve(1); Just channel_data_ = std::vector(1, data_.get()) ? https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer... media/base/audio_buffer.cc:61: if (data) This should never be null now right? https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer... media/base/audio_buffer.cc:115: const size_t data_size) { Instead of adding this, if you specify the appropriate frame size per sample format then this works naturally in the existing code. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_bus.h File media/base/audio_bus.h (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_bus.h#... media/base/audio_bus.h:76: void set_data_size(int data_size); hacker_style() methods must be inline; I know we have some on here that aren't, but don't add more. But that said, why are these changes necessary? Last we discussed the RAW formats are all CBR, so you should be able to just construct a single channel AudioBus with frames/4 size (1 byte vs float). Then you shouldn't need to modify this class at all. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_codecs.h File media/base/audio_codecs.h (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_codecs... media/base/audio_codecs.h:35: kCodecRaw = 17, I don't think this is quite what we want. The correct codec_id should be used, but instead of going through the normal decoder we should have a PassthroughAudioDecoder inserted in the decoder list which returns AudioBuffers with a SampleFormat::MP3, SampleFormat::AAC, SampleFormat::EAC3, etc. For unencrypted buffers we should be able to pipe the demuxer output straight into an AudioBuffer. For encrypted buffers, there's no reason the MediaCodecAudioDecoder can't return buffers of that type. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_parame... File media/base/audio_parameters.h (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_parame... media/base/audio_parameters.h:147: bool IsRawFormat() const { Inline method must be hacker_style(). https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_render... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_render... media/base/audio_renderer_mixer.cc:57: raw_input_(nullptr), Why is this necessary? Instead the AudioConverter should be taught about raw streams and it will do the right thing without modification to this code. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_splice... media/base/audio_splicer.cc:186: if (frames_to_fill == 0 || std::abs(frames_to_fill) < kMinGapSize || Raw formats should just skip the sanitizer entirely. Notably this is going to break things like append window trimming in MSE. So authors will have to be very careful to ensure they align their buffers correctly during streaming. +chcunningham. https://codereview.chromium.org/2466463005/diff/20001/media/filters/android/m... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/filters/android/m... media/filters/android/media_codec_audio_decoder.cc:373: if (is_passthrough_) { Didn't we determine that this is all CBR? So we know duration, etc without this? https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:692: if (state_ == kPlaying && buffer->sample_format() != kSampleFormatRaw) { This is incorrect, seeking is going to return the wrong data. You need at least IsBeforeStartTime() and further you'll need some mechanism for reporting the actual start time in cases where you're unable to trim the audio buffer to the correct start time. https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:783: void AudioRendererImpl::SetPlaybackRate(double playback_rate) { Values other than 0 or 1 need to be ignored when outputing in raw.
https://codereview.chromium.org/2466463005/diff/20001/chromecast/common/media... File chromecast/common/media/cast_media_client.h (right): https://codereview.chromium.org/2466463005/diff/20001/chromecast/common/media... chromecast/common/media/cast_media_client.h:35: bool IsSupportedPassthroughAudio(::media::AudioCodec codec) override; On 2016/11/01 23:05:13, DaleCurtis wrote: > IsPassthroughAudioSupported() ? Done. https://codereview.chromium.org/2466463005/diff/20001/media/audio/audio_outpu... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/audio/audio_outpu... media/audio/audio_output_device.cc:27: static const int kMaxFramesPerCompressedAudioBuffer = 4096; On 2016/11/01 23:05:13, DaleCurtis wrote: > This needs to be a property of the AudioParameters class; that said where did > you get this number from? Done, thanks for your help. https://codereview.chromium.org/2466463005/diff/20001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:51: private WorkerThread mWorkerThread; On 2016/11/01 23:05:13, DaleCurtis wrote: > What's the point of this? Using a thread instead of callbacks is going to lead > to being out of sync with playback and is not as efficient as using callbacks. > > https://chromiumcodereview.appspot.com/1525033003/?_ga=1.90949970.914367063.1... > > This one piece is complicated enough to separate into its own code review with > unittests. We should also consider the question of whether it's worth it to > continue using OpenSLES or if we should always use AudioTrack instead. I did try to use callback, but it failed. Please see: http://androidxref.com/7.0.0_r1/xref/frameworks/av/media/libmedia/AudioTrack.... OK, I will create another CL for AudioTrackOutputStream. I am not familiar with the differences between OpenSLES and AudioTrack. Will update the comment once I have more information. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer... media/base/audio_buffer.cc:55: if (sample_format == kSampleFormatRaw) { I refactored the code a little to avoid code duplication. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer... media/base/audio_buffer.cc:58: base::AlignedAlloc(data_size_, kChannelAlignment))); On 2016/11/01 23:05:13, DaleCurtis wrote: > AlignedAlloc shouldn't be necessary in this case. |data_| is defined as: std::unique_ptr<uint8_t, base::AlignedFreeDeleter> data_; Thus, AlignedAlloc is necessary, but kChannelAlignment isn't. However, the minimum alignment is sizeof(void*), not 1. Not sure we really prefer this alignment instead. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer... media/base/audio_buffer.cc:59: channel_data_.reserve(1); On 2016/11/01 23:05:13, DaleCurtis wrote: > Just channel_data_ = std::vector(1, data_.get()) ? Yes, it should work. It's better in terms of line of code, but I can't say the same thing about performance. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer... media/base/audio_buffer.cc:61: if (data) On 2016/11/01 23:05:13, DaleCurtis wrote: > This should never be null now right? In current use cases, yes. But I would prefer to keep the behavior in sync with PCM cases below. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer... media/base/audio_buffer.cc:115: const size_t data_size) { On 2016/11/01 23:05:13, DaleCurtis wrote: > Instead of adding this, if you specify the appropriate frame size per sample > format then this works naturally in the existing code. I see your point, but it would also introduce another parameter. Also, some calculations would become floating numbers, and we have to round these results to avoid precision issues. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_bus.h File media/base/audio_bus.h (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_bus.h#... media/base/audio_bus.h:76: void set_data_size(int data_size); On 2016/11/01 23:05:13, DaleCurtis wrote: > hacker_style() methods must be inline; I know we have some on here that aren't, > but don't add more. But that said, why are these changes necessary? Last we > discussed the RAW formats are all CBR, so you should be able to just construct a > single channel AudioBus with frames/4 size (1 byte vs float). Then you shouldn't > need to modify this class at all. Thanks, I fixed hacker_style() methods. Last time you pointed out that both AC3 and EAC3 are CBR. However, there are more RAW formats: https://developer.android.com/reference/android/media/AudioFormat.html DTS-HD MA is VBR: http://www.audiogurus.com/learn/electronics/dts-hd-master-audio/186 Do we really want to support CBR only? One question, how to get "audio bit rate" in AudioRendererImpl::Initialize()? That would be the key if we want to support CBR only and keep changes minimum. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_codecs.h File media/base/audio_codecs.h (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_codecs... media/base/audio_codecs.h:35: kCodecRaw = 17, On 2016/11/01 23:05:13, DaleCurtis wrote: > I don't think this is quite what we want. The correct codec_id should be used, > but instead of going through the normal decoder we should have a > PassthroughAudioDecoder inserted in the decoder list which returns AudioBuffers > with a SampleFormat::MP3, SampleFormat::AAC, SampleFormat::EAC3, etc. > > For unencrypted buffers we should be able to pipe the demuxer output straight > into an AudioBuffer. For encrypted buffers, there's no reason the > MediaCodecAudioDecoder can't return buffers of that type. Let's focus on encrypted case first, since that's what we have in HBOGo. I agree with you that we can optimize unencrypted case. The major difficulty is that we have to make sure raw audio decoder and raw audio output stream are enabled at the same time. Right now, it is decided on MediaClient::IsPassthroughAudioSupported(), which is not accessible in MediaCodecAudioDecoder. Base on your comments, what I can thing of is creating MojoPassthroughAudioDecoder, which is a wrapper of MojoAudioDecoder. It would use MediaClient::IsPassthroughAudioSupported() to determine whether to create MojoAudioDecoder or just return unsupported. We also need to add one field in AudioDecoderConfig to notify MediaCodecAudioDecoder to use raw decoder or not. Does it sounds good to you? Any suggestion? https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_parame... File media/base/audio_parameters.h (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_parame... media/base/audio_parameters.h:147: bool IsRawFormat() const { On 2016/11/01 23:05:13, DaleCurtis wrote: > Inline method must be hacker_style(). Done. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_render... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_render... media/base/audio_renderer_mixer.cc:57: raw_input_(nullptr), On 2016/11/01 23:05:13, DaleCurtis wrote: > Why is this necessary? Instead the AudioConverter should be taught about raw > streams and it will do the right thing without modification to this code. Just took a shortcut, since none of these functions are applicable to raw stream. I will look into AudioConverter. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_splice... media/base/audio_splicer.cc:186: if (frames_to_fill == 0 || std::abs(frames_to_fill) < kMinGapSize || On 2016/11/01 23:05:13, DaleCurtis wrote: > Raw formats should just skip the sanitizer entirely. Notably this is going to > break things like append window trimming in MSE. So authors will have to be very > careful to ensure they align their buffers correctly during streaming. > +chcunningham. Sorry, I am not sure what to do. Should I move the change to line 154 or 164? https://codereview.chromium.org/2466463005/diff/20001/media/filters/android/m... File media/filters/android/media_codec_audio_decoder.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/filters/android/m... media/filters/android/media_codec_audio_decoder.cc:373: if (is_passthrough_) { On 2016/11/01 23:05:13, DaleCurtis wrote: > Didn't we determine that this is all CBR? So we know duration, etc without this? Could you please advise how to get audio bit rate? Thanks. https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:692: if (state_ == kPlaying && buffer->sample_format() != kSampleFormatRaw) { On 2016/11/01 23:05:13, DaleCurtis wrote: > This is incorrect, seeking is going to return the wrong data. You need at least > IsBeforeStartTime() and further you'll need some mechanism for reporting the > actual start time in cases where you're unable to trim the audio buffer to the > correct start time. Thanks for pointing it out. Mark reporting time as a to-do item. What's the defect we have now? AV is slightly out of sync in the beginning, and AV sync will be recovered after a short period of time, right? https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:783: void AudioRendererImpl::SetPlaybackRate(double playback_rate) { On 2016/11/01 23:05:13, DaleCurtis wrote: > Values other than 0 or 1 need to be ignored when outputing in raw. Done.
https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_render... File media/base/audio_renderer_mixer.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_render... media/base/audio_renderer_mixer.cc:57: raw_input_(nullptr), On 2016/11/01 23:05:13, DaleCurtis wrote: > Why is this necessary? Instead the AudioConverter should be taught about raw > streams and it will do the right thing without modification to this code. You are right, the change is not necessary.
https://codereview.chromium.org/2466463005/diff/20001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:51: private WorkerThread mWorkerThread; On 2016/11/04 at 18:04:24, AndyWu wrote: > On 2016/11/01 23:05:13, DaleCurtis wrote: > > What's the point of this? Using a thread instead of callbacks is going to lead > > to being out of sync with playback and is not as efficient as using callbacks. > > > > https://chromiumcodereview.appspot.com/1525033003/?_ga=1.90949970.914367063.1... > > > > This one piece is complicated enough to separate into its own code review with > > unittests. We should also consider the question of whether it's worth it to > > continue using OpenSLES or if we should always use AudioTrack instead. > > I did try to use callback, but it failed. Please see: > http://androidxref.com/7.0.0_r1/xref/frameworks/av/media/libmedia/AudioTrack.... > > OK, I will create another CL for AudioTrackOutputStream. > I am not familiar with the differences between OpenSLES and AudioTrack. Will update the comment once I have more information. Ugh, no we wouldn't want to use a non-callback based stream for general audio playback. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer... media/base/audio_buffer.cc:115: const size_t data_size) { On 2016/11/04 at 18:04:24, AndyWu wrote: > On 2016/11/01 23:05:13, DaleCurtis wrote: > > Instead of adding this, if you specify the appropriate frame size per sample > > format then this works naturally in the existing code. > > I see your point, but it would also introduce another parameter. > Also, some calculations would become floating numbers, and we have to round these results to avoid precision issues. Hmm, I guess you're saying that we don't always end up with a multiple of 4 for the buffer size? https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:692: if (state_ == kPlaying && buffer->sample_format() != kSampleFormatRaw) { On 2016/11/04 at 18:04:24, AndyWu wrote: > On 2016/11/01 23:05:13, DaleCurtis wrote: > > This is incorrect, seeking is going to return the wrong data. You need at least > > IsBeforeStartTime() and further you'll need some mechanism for reporting the > > actual start time in cases where you're unable to trim the audio buffer to the > > correct start time. > > Thanks for pointing it out. > Mark reporting time as a to-do item. What's the defect we have now? AV is slightly out of sync in the beginning, and AV sync will be recovered after a short period of time, right? No, AV sync will never recover for this case since audio renderer will think time T-x is actually time T -- so a todo is insufficient here.
https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer... media/base/audio_buffer.cc:115: const size_t data_size) { On 2016/11/04 at 19:48:29, DaleCurtis wrote: > On 2016/11/04 at 18:04:24, AndyWu wrote: > > On 2016/11/01 23:05:13, DaleCurtis wrote: > > > Instead of adding this, if you specify the appropriate frame size per sample > > > format then this works naturally in the existing code. > > > > I see your point, but it would also introduce another parameter. > > Also, some calculations would become floating numbers, and we have to round these results to avoid precision issues. > > Hmm, I guess you're saying that we don't always end up with a multiple of 4 for the buffer size? Actually, why can't you just define these sample formats to be 1 byte per frame, so the frame_count == byte count. The rest of the code should be correct w/o modification then. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_codecs.h File media/base/audio_codecs.h (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_codecs... media/base/audio_codecs.h:35: kCodecRaw = 17, On 2016/11/04 at 18:04:24, AndyWu wrote: > On 2016/11/01 23:05:13, DaleCurtis wrote: > > I don't think this is quite what we want. The correct codec_id should be used, > > but instead of going through the normal decoder we should have a > > PassthroughAudioDecoder inserted in the decoder list which returns AudioBuffers > > with a SampleFormat::MP3, SampleFormat::AAC, SampleFormat::EAC3, etc. > > > > For unencrypted buffers we should be able to pipe the demuxer output straight > > into an AudioBuffer. For encrypted buffers, there's no reason the > > MediaCodecAudioDecoder can't return buffers of that type. > > Let's focus on encrypted case first, since that's what we have in HBOGo. I agree with you that we can optimize unencrypted case. > > The major difficulty is that we have to make sure raw audio decoder and raw audio output stream are enabled at the same time. Right now, it is decided on MediaClient::IsPassthroughAudioSupported(), which is not accessible in MediaCodecAudioDecoder. > > Base on your comments, what I can thing of is creating MojoPassthroughAudioDecoder, which is a wrapper of MojoAudioDecoder. It would use MediaClient::IsPassthroughAudioSupported() to determine whether to create MojoAudioDecoder or just return unsupported. We also need to add one field in AudioDecoderConfig to notify MediaCodecAudioDecoder to use raw decoder or not. > > Does it sounds good to you? Any suggestion? I think whether Raw is used or not needs to be part of the AudioDecoderConfig, not a codec name. This will allow us to flip in and out of raw mode with a decoder restart if necessary (for future stuff) and avoids polluting the codec enums. Plumbing that change can be part of a separate CL too for easier review. Then you'll get the flag in all the right places. https://codereview.chromium.org/2466463005/diff/60001/content/renderer/media/... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2466463005/diff/60001/content/renderer/media/... content/renderer/media/audio_renderer_mixer_manager.cc:204: media::AudioRendererMixer* mixer = new media::AudioRendererMixer( You'll probably need to do more than this to make sure that the mixer doesn't try to mix two bitstream formats. I.e. every raw stream is 1-1.
https://codereview.chromium.org/2466463005/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2466463005/diff/60001/media/audio/audio_outpu... media/audio/audio_output_device.cc:479: output_bus_->set_is_raw_format(audio_parameters_.IsRawFormat()); What happens if you output zeros when in bitstreaming mode? Is that still silence? If so I think it makes sense to not modify AudioBus and just always send even multiples of 4 bytes in frames. Padding the last buffer with zeros.
tsunghung@chromium.org changed reviewers: + mkwst@chromium.org
https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_buffer... media/base/audio_buffer.cc:115: const size_t data_size) { On 2016/11/04 21:17:33, DaleCurtis wrote: > Actually, why can't you just define these sample formats to be 1 byte per frame, > so the frame_count == byte count. The rest of the code should be correct w/o > modification then. The |data_size_| calculation is based on https://cs.chromium.org/chromium/src/media/base/audio_buffer.cc?l=47 https://cs.chromium.org/chromium/src/media/base/audio_buffer.cc?l=82 In order to make byte per frame to be 1, SampleFormatToBytesPerChannel() has to return floating number, not integer anymore. Thus, the derived calculations have to handle precision issues. We also have to fake the frame_count. That may create issues in audio clock or duration calculations. Thus, providing data_size here seems more straightforward and unambiguous. https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_codecs.h File media/base/audio_codecs.h (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_codecs... media/base/audio_codecs.h:35: kCodecRaw = 17, > I think whether Raw is used or not needs to be part of the AudioDecoderConfig, > not a codec name. This will allow us to flip in and out of raw mode with a > decoder restart if necessary (for future stuff) and avoids polluting the codec > enums. > > Plumbing that change can be part of a separate CL too for easier review. Then > you'll get the flag in all the right places. OK, done. I will create a separate CL for review. https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:692: if (state_ == kPlaying && buffer->sample_format() != kSampleFormatRaw) { On 2016/11/04 19:48:29, DaleCurtis wrote: > On 2016/11/04 at 18:04:24, AndyWu wrote: > > On 2016/11/01 23:05:13, DaleCurtis wrote: > > > This is incorrect, seeking is going to return the wrong data. You need at > least > > > IsBeforeStartTime() and further you'll need some mechanism for reporting the > > > actual start time in cases where you're unable to trim the audio buffer to > the > > > correct start time. > > > > Thanks for pointing it out. > > Mark reporting time as a to-do item. What's the defect we have now? AV is > slightly out of sync in the beginning, and AV sync will be recovered after a > short period of time, right? > > No, AV sync will never recover for this case since audio renderer will think > time T-x is actually time T -- so a todo is insufficient here. I see. I reset start_timestamp_and audio_clock_in PS#5. Thanks a lot.
dalecurtis@chromium.org changed reviewers: + watk@chromium.org
https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:692: if (state_ == kPlaying && buffer->sample_format() != kSampleFormatRaw) { On 2016/11/08 at 00:04:21, AndyWu wrote: > On 2016/11/04 19:48:29, DaleCurtis wrote: > > On 2016/11/04 at 18:04:24, AndyWu wrote: > > > On 2016/11/01 23:05:13, DaleCurtis wrote: > > > > This is incorrect, seeking is going to return the wrong data. You need at > > least > > > > IsBeforeStartTime() and further you'll need some mechanism for reporting the > > > > actual start time in cases where you're unable to trim the audio buffer to > > the > > > > correct start time. > > > > > > Thanks for pointing it out. > > > Mark reporting time as a to-do item. What's the defect we have now? AV is > > slightly out of sync in the beginning, and AV sync will be recovered after a > > short period of time, right? > > > > No, AV sync will never recover for this case since audio renderer will think > > time T-x is actually time T -- so a todo is insufficient here. > > I see. I reset start_timestamp_and audio_clock_in PS#5. Thanks a lot. I don't think this will work. There is code in other layers to prevent the time from going backwards. +watk who is familiar with this.
https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:692: if (state_ == kPlaying && buffer->sample_format() != kSampleFormatRaw) { On 2016/11/08 00:11:47, DaleCurtis wrote: > On 2016/11/08 at 00:04:21, AndyWu wrote: > > On 2016/11/04 19:48:29, DaleCurtis wrote: > > > On 2016/11/04 at 18:04:24, AndyWu wrote: > > > > On 2016/11/01 23:05:13, DaleCurtis wrote: > > > > > This is incorrect, seeking is going to return the wrong data. You need > at > > > least > > > > > IsBeforeStartTime() and further you'll need some mechanism for reporting > the > > > > > actual start time in cases where you're unable to trim the audio buffer > to > > > the > > > > > correct start time. > > > > > > > > Thanks for pointing it out. > > > > Mark reporting time as a to-do item. What's the defect we have now? AV is > > > slightly out of sync in the beginning, and AV sync will be recovered after a > > > short period of time, right? > > > > > > No, AV sync will never recover for this case since audio renderer will think > > > time T-x is actually time T -- so a todo is insufficient here. > > > > I see. I reset start_timestamp_and audio_clock_in PS#5. Thanks a lot. > > I don't think this will work. There is code in other layers to prevent the time > from going backwards. +watk who is familiar with this. PipelineImpl::GetMediaTime() tracks the last reported current time to make sure its callers don't see time go backwards. I can't tell you whether it's necessary though. I never got around to investigating what the implications of removing it are.
https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:692: if (state_ == kPlaying && buffer->sample_format() != kSampleFormatRaw) { On 2016/11/08 19:59:30, watk wrote: > PipelineImpl::GetMediaTime() tracks the last reported current time to make sure > its callers don't see time go backwards. I can't tell you whether it's necessary > though. I never got around to investigating what the implications of removing it > are. Thanks a lot for your information. I guess I can do the same thing in AudioRendererImpl.
+1 to Dale's call for a doc. Thats a good place to discuss some unsettled things like the worker thread and how to support append window trimming. Also, I'm not familiar with some components, so a doc should ease review. Also, +1 to Dale's comments about avoiding the name "Raw" when we're really talking about encoded (E)AC3. Lets reserve "raw" for referring to PCM - https://cs.chromium.org/chromium/src/media/base/audio_parameters.h?rcl=0&l=73 https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_splice... media/base/audio_splicer.cc:186: if (frames_to_fill == 0 || std::abs(frames_to_fill) < kMinGapSize || On 2016/11/04 18:04:24, AndyWu wrote: > On 2016/11/01 23:05:13, DaleCurtis wrote: > > Raw formats should just skip the sanitizer entirely. Notably this is going to > > break things like append window trimming in MSE. So authors will have to be > very > > careful to ensure they align their buffers correctly during streaming. > > +chcunningham. > > Sorry, I am not sure what to do. Should I move the change to line 154 or 164? This whole file is now deleted. But Dale is right about append window trimming (see use case: http://dalecurtis.github.io/llama-demo/index.html) Append window trimming is achieved by discarding decoded frames in audio_discard_helper. I'm not sure how you'll support this in a passthrough scenario.
> Thats a good place to discuss some unsettled things... + implications for WebAudio
Here is the document: https://docs.google.com/a/google.com/document/d/1hVRqp_xUAHr9zAE0qmIE8I16Irq-... Hopefully, it could help reviewers.
https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_splice... File media/base/audio_splicer.cc (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/audio_splice... media/base/audio_splicer.cc:186: if (frames_to_fill == 0 || std::abs(frames_to_fill) < kMinGapSize || On 2016/11/11 19:14:28, chcunningham wrote: > On 2016/11/04 18:04:24, AndyWu wrote: > > On 2016/11/01 23:05:13, DaleCurtis wrote: > > > Raw formats should just skip the sanitizer entirely. Notably this is going > to > > > break things like append window trimming in MSE. So authors will have to be > > very > > > careful to ensure they align their buffers correctly during streaming. > > > +chcunningham. > > > > Sorry, I am not sure what to do. Should I move the change to line 154 or 164? > > This whole file is now deleted. But Dale is right about append window trimming > (see use case: http://dalecurtis.github.io/llama-demo/index.html) > > Append window trimming is achieved by discarding decoded frames in > audio_discard_helper. I'm not sure how you'll support this in a passthrough > scenario. Thanks a lot for the well written article from Dale. We can still discard bitstream in terms of "compressed audio frame", which is 576 PCM frames in MP3. That is, dropping a complete AudioBuffer data should be acceptable in passthrough mode.
Description was changed from ========== Support (E)AC3 passthrough When the connected HDMI receiver supports (E)AC3 passthrough, we can directly pass raw compressed (E)AC3 bitstream to AudioTrack. BUG=613385 ========== to ========== Support (E)AC3 passthrough When the connected HDMI receiver supports (E)AC3 passthrough, we can directly pass raw compressed (E)AC3 bitstream to AudioTrack. BUG=613385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
I've done some more research, here's what I think we should do: - Drop all AudioDecoderConfig/DecoderStream changes for now. We can just always force offload mode when we see AC3/EAC3 codecs received by MediaCodecAudioDecoder and AudioRendererImpl. When we experiment with offload MP3/AAC in the future we can reevaluate. - AC3 and EAC3 both use a 256 byte "sample", so we should set the bytes per sample for this content to be 256 bytes with a range of 1-6 frames per buffer. Likely you don't need to even parse this information, just divide the returned byte count from MediaCodec by 256 (there are no partial frames, so it should always be a perfect multiple -- but dcheck this :) https://github.com/google/ExoPlayer/blob/release-v2/library/src/main/java/com... - Because of the above we can use AudioBuffer:TrimXXX() methods without any changes; this allows us to keep the existing preroll system without modification. We also don't need any AudioBuffer modifications aside from support for the new sample format. - Because the sample sizes are multiples of four and there are no partial samples, we don't need any AudioBus changes, the bus should just always be sized at a frame count of some multiple of 256 / 4. - To keep the AudioClock correct, when we write frames into the AudioBus, we'll multiply by the sample size. We can add utility methods to SampleFormat for this. With these changes I think this CL gets a lot simpler and we (the videostack team) can handle dealing with offload -> non-offload cases for MP3, AAC later. This CL will still need some protections in playbackRate() and when WebAudio is connected though. I haven't thought through the best options here yet.
On 2016/11/16 00:19:32, DaleCurtis wrote: > I've done some more research, here's what I think we should do: > - Drop all AudioDecoderConfig/DecoderStream changes for now. We can just always > force offload mode when we see AC3/EAC3 codecs received by > MediaCodecAudioDecoder and AudioRendererImpl. When we experiment with offload > MP3/AAC in the future we can reevaluate. Thanks for your comments, I will do that. > - AC3 and EAC3 both use a 256 byte "sample", so we should set the bytes per > sample for this content to be 256 bytes with a range of 1-6 frames per buffer. > Likely you don't need to even parse this information, just divide the returned > byte count from MediaCodec by 256 (there are no partial frames, so it should > always be a perfect multiple -- but dcheck this :) > https://github.com/google/ExoPlayer/blob/release-v2/library/src/main/java/com... I think you misunderstood. One AC3 syncframe contains 1536(256*6) PCM frames; one EAC3 syncframe contains 256, 512, 768, or 1536 PCM frames. The size of a syncframe is not necessary multiple of 256 bytes; it depends on the bit rate.
On 2016/11/16 at 21:01:47, tsunghung wrote: > On 2016/11/16 00:19:32, DaleCurtis wrote: > > I've done some more research, here's what I think we should do: > > - Drop all AudioDecoderConfig/DecoderStream changes for now. We can just always > > force offload mode when we see AC3/EAC3 codecs received by > > MediaCodecAudioDecoder and AudioRendererImpl. When we experiment with offload > > MP3/AAC in the future we can reevaluate. > > Thanks for your comments, I will do that. > > > - AC3 and EAC3 both use a 256 byte "sample", so we should set the bytes per > > sample for this content to be 256 bytes with a range of 1-6 frames per buffer. > > Likely you don't need to even parse this information, just divide the returned > > byte count from MediaCodec by 256 (there are no partial frames, so it should > > always be a perfect multiple -- but dcheck this :) > > https://github.com/google/ExoPlayer/blob/release-v2/library/src/main/java/com... > > I think you misunderstood. One AC3 syncframe contains 1536(256*6) PCM frames; > one EAC3 syncframe contains 256, 512, 768, or 1536 PCM frames. The size of a > syncframe is not necessary multiple of 256 bytes; it depends on the bit rate. Ah you're right, I'll give it some more thought.
On 2016/11/16 at 22:11:43, DaleCurtis wrote: > On 2016/11/16 at 21:01:47, tsunghung wrote: > > On 2016/11/16 00:19:32, DaleCurtis wrote: > > > I've done some more research, here's what I think we should do: > > > - Drop all AudioDecoderConfig/DecoderStream changes for now. We can just always > > > force offload mode when we see AC3/EAC3 codecs received by > > > MediaCodecAudioDecoder and AudioRendererImpl. When we experiment with offload > > > MP3/AAC in the future we can reevaluate. > > > > Thanks for your comments, I will do that. > > > > > - AC3 and EAC3 both use a 256 byte "sample", so we should set the bytes per > > > sample for this content to be 256 bytes with a range of 1-6 frames per buffer. > > > Likely you don't need to even parse this information, just divide the returned > > > byte count from MediaCodec by 256 (there are no partial frames, so it should > > > always be a perfect multiple -- but dcheck this :) > > > https://github.com/google/ExoPlayer/blob/release-v2/library/src/main/java/com... > > > > I think you misunderstood. One AC3 syncframe contains 1536(256*6) PCM frames; > > one EAC3 syncframe contains 256, 512, 768, or 1536 PCM frames. The size of a > > syncframe is not necessary multiple of 256 bytes; it depends on the bit rate. > > Ah you're right, I'll give it some more thought. So after looking at the spec some more, I think you're right that we'll need the size information. There is some 2-byte alignment, but it's not guaranteed under all applications. So I propose that instead we have something like AudioBuffer::CreateCompressedBuffer() which sets the appropriate internal fields, and DCHECKs() if Trim() is used on a compressed buffer. For AudioBus, being a compressed buffer or not should be part of the construction. I.e. don't have a setter, instead WrapMemory should take a is_compressed flag (maybe WrapMemoryCompressed and CreateCompressed?). It should only be accessible via CopyTo() and CopyTo() should enforce that every target AudioBus is compressed as well. I still recommend splitting this up further as we have discussed.
On 2016/11/29 00:07:52, DaleCurtis wrote: > So after looking at the spec some more, I think you're right that we'll need the > size information. There is some 2-byte alignment, but it's not guaranteed under > all applications. So I propose that instead we have something like > AudioBuffer::CreateCompressedBuffer() which sets the appropriate internal > fields, and DCHECKs() if Trim() is used on a compressed buffer. For AudioBus, > being a compressed buffer or not should be part of the construction. I.e. don't > have a setter, instead WrapMemory should take a is_compressed flag (maybe > WrapMemoryCompressed and CreateCompressed?). It should only be accessible via > CopyTo() and CopyTo() should enforce that every target AudioBus is compressed as > well. > > I still recommend splitting this up further as we have discussed. Thanks a lot for your suggestions. I will do that.
Description was changed from ========== Support (E)AC3 passthrough When the connected HDMI receiver supports (E)AC3 passthrough, we can directly pass raw compressed (E)AC3 bitstream to AudioTrack. BUG=613385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Support (E)AC3 passthrough When the connected HDMI receiver supports (E)AC3 passthrough, we can directly pass raw compressed (E)AC3 bitstream to AudioTrack. BUG=613385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
watk@chromium.org changed reviewers: - watk@chromium.org
Description was changed from ========== Support (E)AC3 passthrough When the connected HDMI receiver supports (E)AC3 passthrough, we can directly pass raw compressed (E)AC3 bitstream to AudioTrack. BUG=613385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Support (E)AC3 passthrough When the connected HDMI receiver supports (E)AC3 passthrough, we can directly pass raw compressed (E)AC3 bitstream to AudioTrack. BUG=613385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
watk@chromium.org changed reviewers: - watk@chromium.org
Description was changed from ========== Support (E)AC3 passthrough When the connected HDMI receiver supports (E)AC3 passthrough, we can directly pass raw compressed (E)AC3 bitstream to AudioTrack. BUG=613385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Support (E)AC3 passthrough When the connected HDMI receiver supports (E)AC3 passthrough, we can directly pass raw compressed (E)AC3 bitstream to AudioTrack. BUG=613385 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
I think the reset of the CL is small enough. Please help to review this CL again, thanks.
Sorry haven't had time to review this due to sheriffing. Will try to get to it Thursday.
I'm new to audio buffer queue, so Dale may take a closer look there. https://codereview.chromium.org/2466463005/diff/20001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:51: private WorkerThread mWorkerThread; This is still unaddressed. Not sure what to do. AudioTrack seems push based. When you say you tried before, what were the specifics of your approach and where it went wrong? https://codereview.chromium.org/2466463005/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2466463005/diff/60001/media/audio/audio_outpu... media/audio/audio_output_device.cc:479: output_bus_->set_is_raw_format(audio_parameters_.IsRawFormat()); Also curious about the meaning of zeroes. https://codereview.chromium.org/2466463005/diff/300001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2466463005/diff/300001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:205: input_params.IsBitstreamFormat() ? input_params : mixer_output_params, I would put this logic into GetMixerOutputParams. This will also make the DVLOG(1) below correct. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... File media/base/audio_buffer_queue.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:67: if ((*buffers_.begin())->IsBitstreamFormat()) { This block needs a comment to explain why |frames| is totally ignored in this case (you're just reading the whole of the front buffer) https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:71: scoped_refptr<AudioBuffer> buffer = buffers_.front(); Should you use current_buffer_ here to be consistent with original code below? https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:76: buffer->ReadFrames(buffer->frame_count(), 0, dest->data_size(), dest); Do you mean to pass dest->data_size() here? Shouldn't this be |dest_frame_offset|? https://codereview.chromium.org/2466463005/diff/300001/media/filters/audio_re... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/filters/audio_re... media/filters/audio_renderer_algorithm.cc:162: dest->set_is_bitstream_format(is_bitstream_format_); this block too could use some documentation for future readers about why we skip logic below. https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:896: frames_written += algorithm_->FillBuffer(audio_bus, 0, frames_requested, special passthrough logic needs "why" documentation
https://codereview.chromium.org/2466463005/diff/20001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java (right): https://codereview.chromium.org/2466463005/diff/20001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:51: private WorkerThread mWorkerThread; I think minimally you'll want to keep track of the delay between read frames and played frames and basically stop asking for more data when the delay grows too large. But more than that, you also might have a more dynamic sleep time? Dale might hate these ideas, so wait for him. https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... File media/audio/android/audio_track_output_stream.h (right): https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.h:53: int total_read_frames_; At sample rate of 44.1 you can only do about 13 hours of playback before this overflows. Thats a lot of playback, but given that this is for TVs I'd say its possible you'll see it happen. I'd use an int64 here. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:202: int position = mAudioTrack.getPlaybackHeadPosition(); The documentation mentions this is secretly an unsigned int, so you'll want to cast It also mentions it will periodically overflow... https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/sources/an...
https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:199: private boolean readMoreData() { IIUC, the way this is structured you will lose data when AudioTrack decides it can't handle anymore. The readMoreData returns false, and you sleep, but then when you wake back up you ask the renderer for more which will mean the last bus you attempted to push to AudioTrack gets dropped.
https://codereview.chromium.org/2466463005/diff/300001/content/browser/render... File content/browser/renderer_host/media/audio_sync_reader.cc (right): https://codereview.chromium.org/2466463005/diff/300001/content/browser/render... content/browser/renderer_host/media/audio_sync_reader.cc:214: if (mute_audio_) Just return instead of having else block. https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... File media/audio/android/audio_track_output_stream.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:31: callback_(nullptr), Inline initialize these parameters in the header file. https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:36: tick_clock_(new base::DefaultTickClock()) { No need for this unless you're going to add a test for this class? https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:118: int total_played_frames) { As Chris notes, this should be an int64_t. https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:122: int delay_in_frame = total_read_frames_ - total_played_frames; Ditto; int64_t. https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:123: if (delay_in_frame < 0) Should be impossible?, DCHECK instead? https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:125: base::TimeDelta delay = base::TimeDelta::FromSecondsD( Use AudioTimestampHelper::FramesToTime(). https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:135: void* native_bus = env->GetDirectBufferAddress(audio_data); Get this pointer ahead of time and use AudioBus::WrapMemory() for the bus passed into OnMoreData() to avoid the extra copy? Requires that the data block be sized appropriately... https://codereview.chromium.org/2466463005/diff/300001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/audio/audio_outp... media/audio/audio_output_device.cc:497: output_bus_->set_is_bitstream_format(audio_parameters_.IsBitstreamFormat()); if (audio_parameters_.IsBitstreamFormat()) { output_bus_->set_is_bitstream_format(true); output_bus_->set_data_size(0); output_bus_->set_frames(audio_parameters_.frames_per_buffer()); } https://codereview.chromium.org/2466463005/diff/300001/media/audio/audio_outp... media/audio/audio_output_device.cc:512: buffer->params.frames = output_bus_->frames(); Hmm, this is always audio_parameters_.frames_per_buffer() ? Is that a mistake? Otherwise there's no point in sending this information over. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:39: class WorkerThread extends Thread { I think instead of doing the threading in Java you should handle it in the C++ class and change this class to expose only a WriteAudio() call that can be called from C++. We have a class FakeAudioWorker (it's named poorly, sorry) which can handle callback scheduling for you. Essentially you'd just construct a FakeAudioWorker and give it your stream information (sample rate, buffer size, etc) and it'll poll at the correct period on existing threads; no need for the sleep business. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:98: @SuppressWarnings("deprecation") What's deprecated here? https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:123: mBufferSizeInBytes = You might consider 3x buffers here to handle momentary underflow from the renderer. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:126: if (mAudioTrack != null) mAudioTrack.release(); Assert instead, this should not happen. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:186: void setVolume(double volume) { Is this going to work at all for bitstream output? https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:197: } mAudioTrack = nullptr; note once this is called the C++ version is about to destruct. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:230: } else { How common is this? This seems like a high frequency allocation? https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... File media/base/android/java/src/test/org/chromium/media/AudioTrackOutputStreamTest.java (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/test/org/chromium/media/AudioTrackOutputStreamTest.java:1: // Copyright 2017 The Chromium Authors. All rights reserved. Tests should exist for the C++ variant too; although I think you could just delete this one after moving all the complexity into the C++ version. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... File media/base/audio_buffer.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer.cc:248: dest->set_is_bitstream_format(IsBitstreamFormat()); Move into l.251 as set_is_bitstream_format(true); https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... File media/base/audio_buffer_queue.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:38: if (!dest->is_bitstream_format()) Add corresponding dcheck for bitstream formats. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:48: return InternalRead( DCHECK that this is never called with a bitstream format? https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:61: int source_frame_offset, Why do you need any changes here? If calling parameters are correct up and down the chain the existing code should work fine, no? https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:64: if (!buffers_.size()) if buffers_.empty() https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:67: if ((*buffers_.begin())->IsBitstreamFormat()) { On 2017/06/14 at 20:03:08, chcunningham wrote: > This block needs a comment to explain why |frames| is totally ignored in this case (you're just reading the whole of the front buffer) Just buffers_.front()->IsBitstreamFormat() https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_bus.cc File media/base/audio_bus.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_bus.c... media/base/audio_bus.cc:169: if (!is_bitstream_format()) { Why? Just change to CHECK(can_set_channel_data_ || is_bitstream_format())? https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_bus.c... media/base/audio_bus.cc:321: dest->set_is_bitstream_format(is_bitstream_format()); Move inside? I don't think we need to worry about switching between the two buffers do we? https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_bus.h File media/base/audio_bus.h (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_bus.h... media/base/audio_bus.h:72: int data_size() const { return data_size_; } this should be GetBitstreamDataSize() and DCHECK(is_bitstream_format_); ditto for SetBitstreamDataSize(); https://codereview.chromium.org/2466463005/diff/300001/media/filters/audio_re... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/filters/audio_re... media/filters/audio_renderer_algorithm.cc:164: dest->set_data_size(0); Why? https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:37: static const int kMaxFramesPerCompressedAudioBuffer = 4096; Needs explanation; possibly should be stored in AudioParameters. https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:203: // Clamp current media time to the last reported value, this prevents higher We already do this in PipelineImpl. https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:386: MediaClient* media_client = GetMediaClient(); if (auto* mc = GetMediaClient()) is_passthrough_ = mc->IsSupportedBitstreamAudioCodec(codec); else is_passthrough_ = false https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:713: if (buffer->timestamp() < start_timestamp_ && I'd just skip this and seek to the first full buffer after the existing start time. I'm not sure this will work correctly in all cases. https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:819: MEDIA_LOG(ERROR, media_log_) WARNING or INFO, no need for ERROR. https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:896: frames_written += algorithm_->FillBuffer(audio_bus, 0, frames_requested, On 2017/06/14 at 20:03:08, chcunningham wrote: > special passthrough logic needs "why" documentation FYI, this is going to be wrong in cases where the first packet timestamp is far into the future relative to the video. See lines l.900..l.921 -- I suspect this will actually happen in practice and will lead to some AV sync delay, so you should print a media_log warning in this case.
Thanks a lot for all comments, will update this CL by next week.
I was caught by a P0 bug and will have a vacation coming soon. Sorry about that I can't update this CL quickly. On 2017/06/16 15:51:31, AndyWu wrote: > Thanks a lot for all comments, will update this CL by next week.
Please continue the code review here: https://chromium-review.googlesource.com/c/596720 There are still a couple of TBD items, but it would be great if I can get other parts reviewed in the meantime. https://codereview.chromium.org/2466463005/diff/300001/content/browser/render... File content/browser/renderer_host/media/audio_sync_reader.cc (right): https://codereview.chromium.org/2466463005/diff/300001/content/browser/render... content/browser/renderer_host/media/audio_sync_reader.cc:214: if (mute_audio_) On 2017/06/15 21:46:31, DaleCurtis wrote: > Just return instead of having else block. Done. https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... File media/audio/android/audio_track_output_stream.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:31: callback_(nullptr), On 2017/06/15 21:46:31, DaleCurtis wrote: > Inline initialize these parameters in the header file. Done. https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:36: tick_clock_(new base::DefaultTickClock()) { On 2017/06/15 21:46:31, DaleCurtis wrote: > No need for this unless you're going to add a test for this class? Done. https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:118: int total_played_frames) { On 2017/06/15 21:46:31, DaleCurtis wrote: > As Chris notes, this should be an int64_t. AudioTrack.getPlaybackHeadPosition()[1] returns a 32 bits integer. The value really matters is the difference between total_read_frames_ and total_played_frames, which is coming from AudioTrack.getPlaybackHeadPosition(). Thus, it's fine to use 32 bits for both total_read_frames_ and total_played_frames and have them overflow, as long as the difference is smaller than 0x7FFFFFFF, which should be a very safe assumption. Anyway, using int64_t is probably a more clear way to eliminate concerns of overflow. [1] https://developer.android.com/reference/android/media/AudioTrack.html#getPlay... https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:122: int delay_in_frame = total_read_frames_ - total_played_frames; On 2017/06/15 21:46:31, DaleCurtis wrote: > Ditto; int64_t. Done. https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:123: if (delay_in_frame < 0) On 2017/06/15 21:46:31, DaleCurtis wrote: > Should be impossible?, DCHECK instead? Normally, it should not happen. However, it could happen when data underflow. https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:125: base::TimeDelta delay = base::TimeDelta::FromSecondsD( On 2017/06/15 21:46:31, DaleCurtis wrote: > Use AudioTimestampHelper::FramesToTime(). Done. https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:135: void* native_bus = env->GetDirectBufferAddress(audio_data); On 2017/06/15 21:46:31, DaleCurtis wrote: > Get this pointer ahead of time and use AudioBus::WrapMemory() for the bus passed > into OnMoreData() to avoid the extra copy? Requires that the data block be sized > appropriately... TBD https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... File media/audio/android/audio_track_output_stream.h (right): https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.h:53: int total_read_frames_; On 2017/06/14 20:27:21, chcunningham wrote: > At sample rate of 44.1 you can only do about 13 hours of playback before this > overflows. Thats a lot of playback, but given that this is for TVs I'd say its > possible you'll see it happen. I'd use an int64 here. Done. https://codereview.chromium.org/2466463005/diff/300001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/audio/audio_outp... media/audio/audio_output_device.cc:497: output_bus_->set_is_bitstream_format(audio_parameters_.IsBitstreamFormat()); On 2017/06/15 21:46:31, DaleCurtis wrote: > if (audio_parameters_.IsBitstreamFormat()) { > output_bus_->set_is_bitstream_format(true); > output_bus_->set_data_size(0); > output_bus_->set_frames(audio_parameters_.frames_per_buffer()); > } Done. https://codereview.chromium.org/2466463005/diff/300001/media/audio/audio_outp... media/audio/audio_output_device.cc:512: buffer->params.frames = output_bus_->frames(); On 2017/06/15 21:46:31, DaleCurtis wrote: > Hmm, this is always audio_parameters_.frames_per_buffer() ? Is that a mistake? > Otherwise there's no point in sending this information over. To make things clear, I changed to GetBitstreamFrames(). It should be PCM frame count of a compressed audio frame. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:39: class WorkerThread extends Thread { On 2017/06/15 21:46:32, DaleCurtis wrote: > I think instead of doing the threading in Java you should handle it in the C++ > class and change this class to expose only a WriteAudio() call that can be > called from C++. > > We have a class FakeAudioWorker (it's named poorly, sorry) which can handle > callback scheduling for you. Essentially you'd just construct a FakeAudioWorker > and give it your stream information (sample rate, buffer size, etc) and it'll > poll at the correct period on existing threads; no need for the sleep business. TBD Will look into that. However, from test point of view, it's easier for me to test Java with Robolectric AudioTrack stub and verify the behavior. Running on top of a real android device probably won't help much, since the device has to connecting to a AC3 enabled sink device. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:98: @SuppressWarnings("deprecation") On 2017/06/15 21:46:32, DaleCurtis wrote: > What's deprecated here? AudioFormat.CHANNEL_OUT_7POINT1 https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:123: mBufferSizeInBytes = On 2017/06/15 21:46:32, DaleCurtis wrote: > You might consider 3x buffers here to handle momentary underflow from the > renderer. Done. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:126: if (mAudioTrack != null) mAudioTrack.release(); On 2017/06/15 21:46:32, DaleCurtis wrote: > Assert instead, this should not happen. Done. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:186: void setVolume(double volume) { On 2017/06/15 21:46:32, DaleCurtis wrote: > Is this going to work at all for bitstream output? Good catch! It does not work for bitstream output. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:197: } On 2017/06/15 21:46:32, DaleCurtis wrote: > mAudioTrack = nullptr; note once this is called the C++ version is about to > destruct. Done. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:199: private boolean readMoreData() { On 2017/06/14 20:32:19, chcunningham wrote: > IIUC, the way this is structured you will lose data when AudioTrack decides it > can't handle anymore. The readMoreData returns false, and you sleep, but then > when you wake back up you ask the renderer for more which will mean the last bus > you attempted to push to AudioTrack gets dropped. You are right. However, I am using blocking version of AudioTrack.write(). The write will normally block until all the data has been enqueued for playback. I don't expect partial write here. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:202: int position = mAudioTrack.getPlaybackHeadPosition(); On 2017/06/14 20:27:21, chcunningham wrote: > The documentation mentions this is secretly an unsigned int, so you'll want to > cast > > It also mentions it will periodically overflow... > > > https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/sources/an... > > Done. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:230: } else { On 2017/06/15 21:46:32, DaleCurtis wrote: > How common is this? This seems like a high frequency allocation? Done. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... File media/base/android/java/src/test/org/chromium/media/AudioTrackOutputStreamTest.java (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/test/org/chromium/media/AudioTrackOutputStreamTest.java:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/06/15 21:46:32, DaleCurtis wrote: > Tests should exist for the C++ variant too; although I think you could just > delete this one after moving all the complexity into the C++ version. TBD. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... File media/base/audio_buffer.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer.cc:248: dest->set_is_bitstream_format(IsBitstreamFormat()); On 2017/06/15 21:46:32, DaleCurtis wrote: > Move into l.251 as set_is_bitstream_format(true); Not really sure it's a good idea to assume the state of the parameter |dest|. I would rather to ensure the state consistency. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... File media/base/audio_buffer_queue.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:38: if (!dest->is_bitstream_format()) On 2017/06/15 21:46:32, DaleCurtis wrote: > Add corresponding dcheck for bitstream formats. Done. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:48: return InternalRead( On 2017/06/15 21:46:32, DaleCurtis wrote: > DCHECK that this is never called with a bitstream format? Why do we need this restriction? https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:61: int source_frame_offset, On 2017/06/15 21:46:32, DaleCurtis wrote: > Why do you need any changes here? If calling parameters are correct up and down > the chain the existing code should work fine, no? For compressed bitstream formats, a partial compressed audio frame is less useful, since it can't generate any PCM frame out of it. Also, we want to keep the granularity as fine as possible so that discarding a small chunk of PCM frames is still possible. Thus, we only transfer a complete AudioBuffer at a time. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:64: if (!buffers_.size()) On 2017/06/15 21:46:32, DaleCurtis wrote: > if buffers_.empty() Done. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:67: if ((*buffers_.begin())->IsBitstreamFormat()) { On 2017/06/14 20:03:08, chcunningham wrote: > This block needs a comment to explain why |frames| is totally ignored in this > case (you're just reading the whole of the front buffer) Done. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:67: if ((*buffers_.begin())->IsBitstreamFormat()) { On 2017/06/15 21:46:32, DaleCurtis wrote: > On 2017/06/14 at 20:03:08, chcunningham wrote: > > This block needs a comment to explain why |frames| is totally ignored in this > case (you're just reading the whole of the front buffer) > > Just buffers_.front()->IsBitstreamFormat() Done. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:71: scoped_refptr<AudioBuffer> buffer = buffers_.front(); On 2017/06/14 20:03:08, chcunningham wrote: > Should you use current_buffer_ here to be consistent with original code below? Done. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:76: buffer->ReadFrames(buffer->frame_count(), 0, dest->data_size(), dest); On 2017/06/14 20:03:08, chcunningham wrote: > Do you mean to pass dest->data_size() here? Shouldn't this be > |dest_frame_offset|? You are right! I was trying to append the data to the end of existing data, so I passed data_size() instead of |dest_frame_offset|. It's tricky and bad. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_bus.cc File media/base/audio_bus.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_bus.c... media/base/audio_bus.cc:169: if (!is_bitstream_format()) { On 2017/06/15 21:46:32, DaleCurtis wrote: > Why? Just change to CHECK(can_set_channel_data_ || is_bitstream_format())? Done. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_bus.c... media/base/audio_bus.cc:321: dest->set_is_bitstream_format(is_bitstream_format()); On 2017/06/15 21:46:32, DaleCurtis wrote: > Move inside? I don't think we need to worry about switching between the two > buffers do we? Not sure it's a good idea to assume the state of a parameter |dest|. I would rather to ensure the state consistency. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_bus.h File media/base/audio_bus.h (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_bus.h... media/base/audio_bus.h:72: int data_size() const { return data_size_; } On 2017/06/15 21:46:32, DaleCurtis wrote: > this should be GetBitstreamDataSize() and DCHECK(is_bitstream_format_); ditto > for SetBitstreamDataSize(); Done. https://codereview.chromium.org/2466463005/diff/300001/media/filters/audio_re... File media/filters/audio_renderer_algorithm.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/filters/audio_re... media/filters/audio_renderer_algorithm.cc:162: dest->set_is_bitstream_format(is_bitstream_format_); On 2017/06/14 20:03:08, chcunningham wrote: > this block too could use some documentation for future readers about why we skip > logic below. Done. https://codereview.chromium.org/2466463005/diff/300001/media/filters/audio_re... media/filters/audio_renderer_algorithm.cc:164: dest->set_data_size(0); On 2017/06/15 21:46:32, DaleCurtis wrote: > Why? Removed. https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:37: static const int kMaxFramesPerCompressedAudioBuffer = 4096; On 2017/06/15 21:46:33, DaleCurtis wrote: > Needs explanation; possibly should be stored in AudioParameters. Done. https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:203: // Clamp current media time to the last reported value, this prevents higher On 2017/06/15 21:46:33, DaleCurtis wrote: > We already do this in PipelineImpl. Hi Chris, do you agree to remove this logic? https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:386: MediaClient* media_client = GetMediaClient(); On 2017/06/15 21:46:33, DaleCurtis wrote: > if (auto* mc = GetMediaClient()) > is_passthrough_ = mc->IsSupportedBitstreamAudioCodec(codec); > else > is_passthrough_ = false Done. https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:713: if (buffer->timestamp() < start_timestamp_ && On 2017/06/15 21:46:33, DaleCurtis wrote: > I'd just skip this and seek to the first full buffer after the existing start > time. I'm not sure this will work correctly in all cases. Done. https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:819: MEDIA_LOG(ERROR, media_log_) On 2017/06/15 21:46:32, DaleCurtis wrote: > WARNING or INFO, no need for ERROR. Done. https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:896: frames_written += algorithm_->FillBuffer(audio_bus, 0, frames_requested, On 2017/06/14 20:03:08, chcunningham wrote: > special passthrough logic needs "why" documentation Done. https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:896: frames_written += algorithm_->FillBuffer(audio_bus, 0, frames_requested, On 2017/06/15 21:46:33, DaleCurtis wrote: > On 2017/06/14 at 20:03:08, chcunningham wrote: > > special passthrough logic needs "why" documentation > > FYI, this is going to be wrong in cases where the first packet timestamp is far > into the future relative to the video. See lines l.900..l.921 -- I suspect this > will actually happen in practice and will lead to some AV sync delay, so you > should print a media_log warning in this case. Done.
https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... File media/audio/android/audio_track_output_stream.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:118: int total_played_frames) { On 2017/08/02 at 01:43:38, AndyWu wrote: > On 2017/06/15 21:46:31, DaleCurtis wrote: > > As Chris notes, this should be an int64_t. > > AudioTrack.getPlaybackHeadPosition()[1] returns a 32 bits integer. The value really matters is the difference between total_read_frames_ and total_played_frames, which is coming from AudioTrack.getPlaybackHeadPosition(). Thus, it's fine to use 32 bits for both total_read_frames_ and total_played_frames and have them overflow, as long as the difference is smaller than 0x7FFFFFFF, which should be a very safe assumption. > > Anyway, using int64_t is probably a more clear way to eliminate concerns of overflow. > > [1] https://developer.android.com/reference/android/media/AudioTrack.html#getPlay... Overflow isn't defined for signed types, so if you expect these to overflow I'd upcast to uint64_t so that's unlikely. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... File media/base/audio_buffer_queue.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:61: int source_frame_offset, On 2017/08/02 at 01:43:40, AndyWu wrote: > On 2017/06/15 21:46:32, DaleCurtis wrote: > > Why do you need any changes here? If calling parameters are correct up and down > > the chain the existing code should work fine, no? > > For compressed bitstream formats, a partial compressed audio frame is less useful, since it can't generate any PCM frame out of it. Also, we want to keep the granularity as fine as possible so that discarding a small chunk of PCM frames is still possible. Thus, we only transfer a complete AudioBuffer at a time. I guess I meant why not just DCHECK() that you have the right parameters when a bitstream format is used.
https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... File media/audio/android/audio_track_output_stream.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:118: int total_played_frames) { On 2017/08/03 01:38:10, DaleCurtis wrote: > On 2017/08/02 at 01:43:38, AndyWu wrote: > > On 2017/06/15 21:46:31, DaleCurtis wrote: > > > As Chris notes, this should be an int64_t. > > > > AudioTrack.getPlaybackHeadPosition()[1] returns a 32 bits integer. The value > really matters is the difference between total_read_frames_ and > total_played_frames, which is coming from AudioTrack.getPlaybackHeadPosition(). > Thus, it's fine to use 32 bits for both total_read_frames_ and > total_played_frames and have them overflow, as long as the difference is smaller > than 0x7FFFFFFF, which should be a very safe assumption. > > > > Anyway, using int64_t is probably a more clear way to eliminate concerns of > overflow. > > > > [1] > https://developer.android.com/reference/android/media/AudioTrack.html#getPlay... > > Overflow isn't defined for signed types, so if you expect these to overflow I'd > upcast to uint64_t so that's unlikely. Well, I consider 0x7FFFFFFF + 1 = 0x80000000 is overflowing for a signed 32-bit integer, since the value changes from 2147483647 to -2147483648. What I was trying to explain is that it's fine to have |total_read_frames_| and |total_played_frames| overflow here. Some examples: 1) When |total_read_frames_| is 0x7FFFFFFA and |total_played_frames| is 0x7FFFFFEA, |delay_in_frame| = 0x7FFFFFFA - 0x7FFFFFEA = 16. 2) When |total_read_frames_| is 0x8000000A and |total_played_frames| is 0x7FFFFFFA, |delay_in_frame| = 0x8000000A - 0x7FFFFFFA = 16. 3) When |total_read_frames_| is 0xFFFFFFFA and |total_played_frames| is 0xFFFFFFEA, |delay_in_frame| = 0xFFFFFFFA - 0xFFFFFFEA = 16. 4) When |total_read_frames_| is 0x0000000A and |total_played_frames| is 0xFFFFFFFA, |delay_in_frame| = 0x0000000A - 0xFFFFFFFA = 16. Anyway, I already changed them to 64-bit integer in https://chromium-review.googlesource.com/c/596720, so they are unlikely to overflow. https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... File media/base/audio_buffer_queue.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/audio_buffe... media/base/audio_buffer_queue.cc:61: int source_frame_offset, On 2017/08/03 01:38:10, DaleCurtis wrote: > On 2017/08/02 at 01:43:40, AndyWu wrote: > > On 2017/06/15 21:46:32, DaleCurtis wrote: > > > Why do you need any changes here? If calling parameters are correct up and > down > > > the chain the existing code should work fine, no? > > > > For compressed bitstream formats, a partial compressed audio frame is less > useful, since it can't generate any PCM frame out of it. Also, we want to keep > the granularity as fine as possible so that discarding a small chunk of PCM > frames is still possible. Thus, we only transfer a complete AudioBuffer at a > time. > > I guess I meant why not just DCHECK() that you have the right parameters when a > bitstream format is used. Unfortunately, we don't know the right parameter during initialization stage [1], so I use a reasonable large number, kMaxFramesPerCompressedAudioBuffer, to only ensure the capacity of |dest|. Thus, the frame count of |dest| won't be the same as the frame count of a AudioBuffer. [1] https://chromium-review.googlesource.com/c/596720/2/media/renderers/audio_ren...
https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... File media/audio/android/audio_track_output_stream.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:118: int total_played_frames) { On 2017/08/03 at 17:11:17, AndyWu wrote: > On 2017/08/03 01:38:10, DaleCurtis wrote: > > On 2017/08/02 at 01:43:38, AndyWu wrote: > > > On 2017/06/15 21:46:31, DaleCurtis wrote: > > > > As Chris notes, this should be an int64_t. > > > > > > AudioTrack.getPlaybackHeadPosition()[1] returns a 32 bits integer. The value > > really matters is the difference between total_read_frames_ and > > total_played_frames, which is coming from AudioTrack.getPlaybackHeadPosition(). > > Thus, it's fine to use 32 bits for both total_read_frames_ and > > total_played_frames and have them overflow, as long as the difference is smaller > > than 0x7FFFFFFF, which should be a very safe assumption. > > > > > > Anyway, using int64_t is probably a more clear way to eliminate concerns of > > overflow. > > > > > > [1] > > https://developer.android.com/reference/android/media/AudioTrack.html#getPlay... > > > > Overflow isn't defined for signed types, so if you expect these to overflow I'd > > upcast to uint64_t so that's unlikely. > > Well, I consider 0x7FFFFFFF + 1 = 0x80000000 is overflowing for a signed 32-bit integer, since the value changes from 2147483647 to -2147483648. What I was trying to explain is that it's fine to have |total_read_frames_| and |total_played_frames| overflow here. Some examples: > 1) When |total_read_frames_| is 0x7FFFFFFA and |total_played_frames| is 0x7FFFFFEA, |delay_in_frame| = 0x7FFFFFFA - 0x7FFFFFEA = 16. > 2) When |total_read_frames_| is 0x8000000A and |total_played_frames| is 0x7FFFFFFA, |delay_in_frame| = 0x8000000A - 0x7FFFFFFA = 16. > 3) When |total_read_frames_| is 0xFFFFFFFA and |total_played_frames| is 0xFFFFFFEA, |delay_in_frame| = 0xFFFFFFFA - 0xFFFFFFEA = 16. > 4) When |total_read_frames_| is 0x0000000A and |total_played_frames| is 0xFFFFFFFA, |delay_in_frame| = 0x0000000A - 0xFFFFFFFA = 16. > > Anyway, I already changed them to 64-bit integer in https://chromium-review.googlesource.com/c/596720, so they are unlikely to overflow. Thanks for changing it. For future reference, what I mean by undefined behavior (and what it typically refers to) is that the compiler may behave in undefined ways. To wit, most compilers take advantage of signed overflow being undefined to make some optimizations: https://kristerw.blogspot.com/2016/02/how-undefined-signed-overflow-enables.html Depending on what you're doing the compiler may do something that totally breaks the assumptions you've stated above.
https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... File media/audio/android/audio_track_output_stream.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/audio/android/au... media/audio/android/audio_track_output_stream.cc:118: int total_played_frames) { On 2017/08/03 17:22:55, DaleCurtis wrote: > Thanks for changing it. For future reference, what I mean by undefined behavior > (and what it typically refers to) is that the compiler may behave in undefined > ways. To wit, most compilers take advantage of signed overflow being undefined > to make some optimizations: > > https://kristerw.blogspot.com/2016/02/how-undefined-signed-overflow-enables.html > > Depending on what you're doing the compiler may do something that totally breaks > the assumptions you've stated above. Thanks for the great information. :) It does make overflow a danger behavior.
Getting closer! Still standing by for the threading to move to c++ and use callbacks. https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:199: private boolean readMoreData() { On 2017/08/02 01:43:39, AndyWu wrote: > On 2017/06/14 20:32:19, chcunningham wrote: > > IIUC, the way this is structured you will lose data when AudioTrack decides it > > can't handle anymore. The readMoreData returns false, and you sleep, but then > > when you wake back up you ask the renderer for more which will mean the last > bus > > you attempted to push to AudioTrack gets dropped. > > You are right. However, I am using blocking version of AudioTrack.write(). The > write will normally block until all the data has been enqueued for playback. I > don't expect partial write here. From my read of the docs, it seems even the blocking mode for write can be interrupted/cut-short for a number of reasons. See all the "or" cases in the snip below. """However, if the write mode is WRITE_NON_BLOCKING, or the track is stopped or paused on entry, or another thread interrupts the write by calling stop or pause, or an I/O error occurs during the write, then the write may return a short transfer count.""" https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:203: // Clamp current media time to the last reported value, this prevents higher On 2017/08/02 01:43:41, AndyWu wrote: > On 2017/06/15 21:46:33, DaleCurtis wrote: > > We already do this in PipelineImpl. > > Hi Chris, do you agree to remove this logic? Yep, should be fine. It used to live here, but had to move for reasons I can't recall. Basically, users of this renderer are already savvy about it moving time backward. I do appreciate the documentation though. You might add this snip about compressed audio to the .h file comments for this method.
https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:199: private boolean readMoreData() { On 2017/08/04 19:26:40, chcunningham wrote: > > From my read of the docs, it seems even the blocking mode for write can be > interrupted/cut-short for a number of reasons. See all the "or" cases in the > snip below. > > """However, if the write mode is WRITE_NON_BLOCKING, or the track is stopped or > paused on entry, or another thread interrupts the write by calling stop or > pause, or an I/O error occurs during the write, then the write may return a > short transfer count.""" Yes, you are right. See AudioTrackOutputStream.close(), we interrupt and terminate the worker thread, then pause the AudioTrack. 1) AudioTrack should be in the play state when AudioTrack.write() is executing. I don't expect any other state. 2) When AudioTrack.write() is interrupted, it means the AudioTrackOutputStream is closing. Thus, there is no need to preserve unwritten data. 3) When I/O error occurs, preserve unwritten data probably won't help much either. Anyway, you are right. If you insist, I will be happy to preserve unwritten data and retry. I just couldn't see a good reason. https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2466463005/diff/300001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:203: // Clamp current media time to the last reported value, this prevents higher On 2017/08/04 19:26:40, chcunningham wrote: > On 2017/08/02 01:43:41, AndyWu wrote: > > On 2017/06/15 21:46:33, DaleCurtis wrote: > > > We already do this in PipelineImpl. > > > > Hi Chris, do you agree to remove this logic? > > Yep, should be fine. It used to live here, but had to move for reasons I can't > recall. Basically, users of this renderer are already savvy about it moving time > backward. > > I do appreciate the documentation though. You might add this snip about > compressed audio to the .h file comments for this method. Thanks for your feedback.
https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java (right): https://codereview.chromium.org/2466463005/diff/300001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioTrackOutputStream.java:199: private boolean readMoreData() { On 2017/06/14 20:32:19, chcunningham wrote: > IIUC, the way this is structured you will lose data when AudioTrack decides it > can't handle anymore. The readMoreData returns false, and you sleep, but then > when you wake back up you ask the renderer for more which will mean the last bus > you attempted to push to AudioTrack gets dropped. Done, thanks. media/base/android/java/src/test/org/chromium/media/AudioTrackOutputStreamTest.java |
