Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
805273007:
Android: Propagate sample rate change to audio decoder job (Closed)
|
Created:
5 years, 11 months ago by kjoswiak Modified:
5 years, 11 months ago CC:
chromium-reviews, posciak+watch_chromium.org, wjia+watch_chromium.org, avayvod+watch_chromium.org, mcasas+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid: Propagate sample rate change to audio decoder job
AudioDecoderJob maintains an object audio_timestamp_helper_ for use with AV sync. Previously, this object was configured with demuxer value for sample rate, whereas it should instead be synced with the decoder's reported sample rate.
This CL enables AudioDecoderJob to track decoder output rate, and ensures change in output format is propagated to this object.
BUG=internal b/18873488
Committed: https://crrev.com/aa9cc0db413c376590cf974b52b847a46dd14c77
Cr-Commit-Position: refs/heads/master@{#313416}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Moved reconfigureAudioTrack into UpdateOutputFormat() codepath #
Total comments: 10
Patch Set 3 : Moved UpdateOutput() back to original location #
Total comments: 8
Patch Set 4 : Minor fixes #Patch Set 5 : Moved reconfigure of audio track back up to java side, moved reconfiguring of timestamp to the Dequ… #
Total comments: 4
Patch Set 6 : Use setPlaybackRate instead of reconstructing track #
Total comments: 3
Patch Set 7 : Remove reconfigureAudioTrack #
Total comments: 8
Patch Set 8 : #
Total comments: 2
Patch Set 9 : Restore deleted comment #Patch Set 10 : #
Messages
Total messages: 44 (6 generated)
kjoswiak@chromium.org changed reviewers: + gunsch@chromium.org, qinmin@chromium.org
Overall structure looks good, but obviously interested to hear Min's thoughts. https://codereview.chromium.org/805273007/diff/1/media/base/android/audio_dec... File media/base/android/audio_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/1/media/base/android/audio_dec... media/base/android/audio_decoder_job.cc:168: ResetTimestampHelper(); Thought: it might also be a good idea to call reconfigureAudioTrack from here. With this addition and the previous CL in MediaCodecBridge.java, there are now two paths for reconfiguring AudioDecoderJob on a STATUS_FORMAT_CHANGED: 1) (this class) UpdateOutputFormat --> ResetTimestampHelper 2) (MediaCodecBridge.java) dequeueOutputBuffer --> reconfigureAudioTrack It would probably be better to essentially revert the changes to dequeueOutputBuffer from the previous CL, add a @CalledFromNative to reconfigureAudioTrack, and also invoke that here. https://codereview.chromium.org/805273007/diff/1/media/base/android/media_dec... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:448: else if (has_format_change && UpdateOutputFormat()) What's the reason for moving UpdateOutputFormat up here?
https://codereview.chromium.org/805273007/diff/1/media/base/android/audio_dec... File media/base/android/audio_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/1/media/base/android/audio_dec... media/base/android/audio_decoder_job.cc:168: ResetTimestampHelper(); On 2015/01/12 16:30:51, gunsch wrote: > Thought: it might also be a good idea to call reconfigureAudioTrack from here. > > With this addition and the previous CL in MediaCodecBridge.java, there are now > two paths for reconfiguring AudioDecoderJob on a STATUS_FORMAT_CHANGED: > > 1) (this class) UpdateOutputFormat --> ResetTimestampHelper > 2) (MediaCodecBridge.java) dequeueOutputBuffer --> reconfigureAudioTrack > > It would probably be better to essentially revert the changes to > dequeueOutputBuffer from the previous CL, add a @CalledFromNative to > reconfigureAudioTrack, and also invoke that here. Hmm, maybe. I would have to add/change codepath to handle error condition of failing the reconfigure. Will look into if I can do this cleanly https://codereview.chromium.org/805273007/diff/1/media/base/android/media_dec... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/1/media/base/android/media_dec... media/base/android/media_decoder_job.cc:448: else if (has_format_change && UpdateOutputFormat()) On 2015/01/12 16:30:51, gunsch wrote: > What's the reason for moving UpdateOutputFormat up here? Short story, it crashes without this change. Long story, when we get the format changed information, it means the buffers we just got from decoder have new format. If this call is where it was previously, in the OnDecodeComplete function, we only update the internal state after the Decode process. However, we now need this information to be up to date during the decode (specifically before we run ReleaseOutputBuffer, which is where we render the buffers and do the timestamp helper stuff). Tracking these frames at an incorrect rate, and then suddenly on the next call at the correct one, happens to screw up the state enough to cause check failure. I think there's technically another fix I could do to make the tracking more robust, but I think its best we ensure we are always working with correct information =) Also shouldn't break anything else, as https://codereview.chromium.org/730133003 almost reduced this function to a no-op video side, or at very least negated what appears to be the original intent of this code path.
Changed some things and moved the reconfigure into UpdateOutput, but not sure if I like how things look. Given that the audio track is managed java side of the bridge, I almost feel more comfortable leaving its reconfigure up to that code. Let me know what you think, and if this call should originate c++ side if there's a cleaner way to deal with updating the status after calling UpdateOutputFormat
https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... File media/base/android/media_codec_bridge.cc (right): https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... media/base/android/media_codec_bridge.cc:294: Java_MediaCodecBridge_getOutputSamplingRate(env, j_media_codec_.obj()); 2 more spaces for indent https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... media/base/android/media_decoder_job.cc:448: else if (has_format_change) { This is on the decoder thread, you should never call updateOutputFormat() here as it updates the variables belongs to the main thread. Instead, you should handle the format change when OnDecodeCompleted() is called. https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... media/base/android/media_decoder_job.h:234: // Update the output format from the decoder, returns true if the output update the return value
https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... File media/base/android/media_codec_bridge.cc (right): https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... media/base/android/media_codec_bridge.cc:294: Java_MediaCodecBridge_getOutputSamplingRate(env, j_media_codec_.obj()); On 2015/01/12 23:57:15, qinmin wrote: > 2 more spaces for indent Done. https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... media/base/android/media_decoder_job.cc:448: else if (has_format_change) { On 2015/01/12 23:57:15, qinmin wrote: > This is on the decoder thread, you should never call updateOutputFormat() here > as it updates the variables belongs to the main thread. > > Instead, you should handle the format change when OnDecodeCompleted() is called. Ahh, hmm. See my reply to Jesse's comment in patch 1 as to why I did this. Instead, what if I added status as a parameter to MediaDecoderJob::ReleaseOutputBuffer, and did this logic there? https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... media/base/android/media_decoder_job.h:234: // Update the output format from the decoder, returns true if the output On 2015/01/12 23:57:15, qinmin wrote: > update the return value Done.
https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... media/base/android/media_decoder_job.cc:448: else if (has_format_change) { On 2015/01/13 00:14:30, kjoswiak wrote: > On 2015/01/12 23:57:15, qinmin wrote: > > This is on the decoder thread, you should never call updateOutputFormat() here > > as it updates the variables belongs to the main thread. > > > > Instead, you should handle the format change when OnDecodeCompleted() is > called. > > Ahh, hmm. See my reply to Jesse's comment in patch 1 as to why I did this. > Instead, what if I added status as a parameter to > MediaDecoderJob::ReleaseOutputBuffer, and did this logic there? Looking closer, this fix wouldn't change anything. However, I'm also not sure I understand where the issue is. What main / ui thread variables are we editing? From what I see, the only variables we end up changing belong to the MediaDecoderJob (or more accurately its subclasses), so I would think it should run on MediaDecoderJob's thread, and isn't that decoder_task_runner_ / what we are on?
https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... media/base/android/media_decoder_job.cc:448: else if (has_format_change) { Kyle: check the first line of each function. Some are executed on decoder_task_runner_ (decoder thread), some are executed on ui_task_runner_ (browser main thread).
https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... media/base/android/media_decoder_job.cc:448: else if (has_format_change) { On 2015/01/13 00:39:42, kjoswiak wrote: > On 2015/01/13 00:14:30, kjoswiak wrote: > > On 2015/01/12 23:57:15, qinmin wrote: > > > This is on the decoder thread, you should never call updateOutputFormat() > here > > > as it updates the variables belongs to the main thread. > > > > > > Instead, you should handle the format change when OnDecodeCompleted() is > > called. > > > > Ahh, hmm. See my reply to Jesse's comment in patch 1 as to why I did this. > > Instead, what if I added status as a parameter to > > MediaDecoderJob::ReleaseOutputBuffer, and did this logic there? > > Looking closer, this fix wouldn't change anything. > > However, I'm also not sure I understand where the issue is. What main / ui > thread variables are we editing? From what I see, the only variables we end up > changing belong to the MediaDecoderJob (or more accurately its subclasses), so I > would think it should run on MediaDecoderJob's thread, and isn't that > decoder_task_runner_ / what we are on? you are touching variables like time_stamp_helper, output_width_, output_height_ on the decoder thread. MediaDecoder runs on 2 thread, ui_task_runner and decoder_task_runner_. Some of the variables (format, timestamp, etc) should be maintained on the ui thread, while others are in the decoder thread.
https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... media/base/android/media_decoder_job.cc:448: else if (has_format_change) { On 2015/01/13 00:46:51, qinmin wrote: > On 2015/01/13 00:39:42, kjoswiak wrote: > > On 2015/01/13 00:14:30, kjoswiak wrote: > > > On 2015/01/12 23:57:15, qinmin wrote: > > > > This is on the decoder thread, you should never call updateOutputFormat() > > here > > > > as it updates the variables belongs to the main thread. > > > > > > > > Instead, you should handle the format change when OnDecodeCompleted() is > > > called. > > > > > > Ahh, hmm. See my reply to Jesse's comment in patch 1 as to why I did this. > > > Instead, what if I added status as a parameter to > > > MediaDecoderJob::ReleaseOutputBuffer, and did this logic there? > > > > Looking closer, this fix wouldn't change anything. > > > > However, I'm also not sure I understand where the issue is. What main / ui > > thread variables are we editing? From what I see, the only variables we end up > > changing belong to the MediaDecoderJob (or more accurately its subclasses), so > I > > would think it should run on MediaDecoderJob's thread, and isn't that > > decoder_task_runner_ / what we are on? > > you are touching variables like time_stamp_helper, output_width_, output_height_ > on the decoder thread. > MediaDecoder runs on 2 thread, ui_task_runner and decoder_task_runner_. > Some of the variables (format, timestamp, etc) should be maintained on the ui > thread, while others are in the decoder thread. Hmm I see. We use audio_timestamp_helper_ in ReleaseOutputBuffer, which runs on decoder thread. Is the fact this object can be changed by ui thread while we are feeding it not a concern? Shouldn't we update it in the thread that uses it?
On 2015/01/13 01:05:58, kjoswiak wrote: > https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... > File media/base/android/media_decoder_job.cc (right): > > https://codereview.chromium.org/805273007/diff/20001/media/base/android/media... > media/base/android/media_decoder_job.cc:448: else if (has_format_change) { > On 2015/01/13 00:46:51, qinmin wrote: > > On 2015/01/13 00:39:42, kjoswiak wrote: > > > On 2015/01/13 00:14:30, kjoswiak wrote: > > > > On 2015/01/12 23:57:15, qinmin wrote: > > > > > This is on the decoder thread, you should never call > updateOutputFormat() > > > here > > > > > as it updates the variables belongs to the main thread. > > > > > > > > > > Instead, you should handle the format change when OnDecodeCompleted() is > > > > called. > > > > > > > > Ahh, hmm. See my reply to Jesse's comment in patch 1 as to why I did this. > > > > Instead, what if I added status as a parameter to > > > > MediaDecoderJob::ReleaseOutputBuffer, and did this logic there? > > > > > > Looking closer, this fix wouldn't change anything. > > > > > > However, I'm also not sure I understand where the issue is. What main / ui > > > thread variables are we editing? From what I see, the only variables we end > up > > > changing belong to the MediaDecoderJob (or more accurately its subclasses), > so > > I > > > would think it should run on MediaDecoderJob's thread, and isn't that > > > decoder_task_runner_ / what we are on? > > > > you are touching variables like time_stamp_helper, output_width_, > output_height_ > > on the decoder thread. > > MediaDecoder runs on 2 thread, ui_task_runner and decoder_task_runner_. > > Some of the variables (format, timestamp, etc) should be maintained on the ui > > thread, while others are in the decoder thread. > > Hmm I see. We use audio_timestamp_helper_ in ReleaseOutputBuffer, which runs on > decoder thread. Is the fact this object can be changed by ui thread while we are > feeding it not a concern? Shouldn't we update it in the thread that uses it? Yes, audio_timestamp_helper_ should also be moved to the ui thread.
https://codereview.chromium.org/805273007/diff/40001/media/base/android/audio... File media/base/android/audio_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/40001/media/base/android/audio... media/base/android/audio_decoder_job.cc:169: ResetTimestampHelper(); This function call is still not threadsafe with ReleaseOutputBuffer(), and they now run on different threads. Thinking about best solution, if nothing clever comes to mind will move this call back to decoder thread What would the policy be for putting a lock around object? Is it ok with ui thread?
https://codereview.chromium.org/805273007/diff/40001/media/base/android/audio... File media/base/android/audio_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/40001/media/base/android/audio... media/base/android/audio_decoder_job.cc:169: ResetTimestampHelper(); On 2015/01/16 23:14:56, kjoswiak wrote: > This function call is still not threadsafe with ReleaseOutputBuffer(), and they > now run on different threads. Thinking about best solution, if nothing clever > comes to mind will move this call back to decoder thread > > What would the policy be for putting a lock around object? Is it ok with ui > thread? Ehh I think it's fine since there can only be one decode running anyway. Ready for review
https://codereview.chromium.org/805273007/diff/40001/media/base/android/audio... File media/base/android/audio_decoder_job.h (right): https://codereview.chromium.org/805273007/diff/40001/media/base/android/audio... media/base/android/audio_decoder_job.h:68: // Audio output format s/output format/output sample rate/ https://codereview.chromium.org/805273007/diff/40001/media/base/android/media... File media/base/android/media_codec_bridge.h (right): https://codereview.chromium.org/805273007/diff/40001/media/base/android/media... media/base/android/media_codec_bridge.h:112: void GetOutputSamplingRate(int* sampling_rate); why not int GetOutputSamplingRate() as there is only 1 return value? https://codereview.chromium.org/805273007/diff/40001/media/base/android/video... File media/base/android/video_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/40001/media/base/android/video... media/base/android/video_decoder_job.cc:169: config_changed_cb.Run(); nit: {} required for if statement that spans multiple lines.
Still chasing down one last issue. While single quality change is handled fine, I've found that after a second quality change, BOTH audio and video play back at half rate. Interestingly, does not happen with patch set 1. I suspect it results from delaying the reconfigure of audio track, will look into more https://codereview.chromium.org/805273007/diff/40001/media/base/android/audio... File media/base/android/audio_decoder_job.h (right): https://codereview.chromium.org/805273007/diff/40001/media/base/android/audio... media/base/android/audio_decoder_job.h:68: // Audio output format On 2015/01/21 02:04:13, qinmin wrote: > s/output format/output sample rate/ Done. https://codereview.chromium.org/805273007/diff/40001/media/base/android/media... File media/base/android/media_codec_bridge.h (right): https://codereview.chromium.org/805273007/diff/40001/media/base/android/media... media/base/android/media_codec_bridge.h:112: void GetOutputSamplingRate(int* sampling_rate); On 2015/01/21 02:04:13, qinmin wrote: > why not int GetOutputSamplingRate() as there is only 1 return value? Done. https://codereview.chromium.org/805273007/diff/40001/media/base/android/video... File media/base/android/video_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/40001/media/base/android/video... media/base/android/video_decoder_job.cc:169: config_changed_cb.Run(); On 2015/01/21 02:04:13, qinmin wrote: > nit: {} required for if statement that spans multiple lines. Done.
Ok, things are mostly fixed. There's an issue where after a certain number of quality changes (~5) a noticeable amount of a/v sync error accumulates. I'm not sure if this results from this CL, or if its a separate issue.
On 2015/01/21 20:25:08, kjoswiak wrote: > Ok, things are mostly fixed. There's an issue where after a certain number of > quality changes (~5) a noticeable amount of a/v sync error accumulates. I'm not > sure if this results from this CL, or if its a separate issue. There a couple issues i can think of: 1. If you want to switch AudioTrack, you need to be sure that the current AudioTrack has consumed all the data. Otherwise, you will notice the glitches. Even if all the data in the previous AudioTrack are completely consumed, things might still not be smooth when you switch to another AudioTrack. 2. When resetting the AudioTimestampHelper, not only the base timestamp needs to be passed, but also the max timestamp since the AudioTrack may not be empty.
https://codereview.chromium.org/805273007/diff/80001/media/base/android/audio... File media/base/android/audio_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/80001/media/base/android/audio... media/base/android/audio_decoder_job.cc:85: audio_timestamp_helper_->SetBaseTimestamp(base_timestamp_); what is the max timestamp here? the new audio_timestamp_helper_ doesn't inherit the max timestamp from the old one
https://codereview.chromium.org/805273007/diff/80001/media/base/android/audio... File media/base/android/audio_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/80001/media/base/android/audio... media/base/android/audio_decoder_job.cc:85: audio_timestamp_helper_->SetBaseTimestamp(base_timestamp_); On 2015/01/21 21:43:56, qinmin wrote: > what is the max timestamp here? the new audio_timestamp_helper_ doesn't inherit > the max timestamp from the old one What do you mean? We preserve storage of time by transferring over the base timestamp, so computation of max_timestamp as we pass up callback will be still be correct. Also, if referring to loss of frame_count information, its alright (actually, required) since we recreate the audio track, and new track will have head position of 0.
https://codereview.chromium.org/805273007/diff/80001/media/base/android/audio... File media/base/android/audio_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/80001/media/base/android/audio... media/base/android/audio_decoder_job.cc:85: audio_timestamp_helper_->SetBaseTimestamp(base_timestamp_); On 2015/01/21 22:53:11, kjoswiak wrote: > On 2015/01/21 21:43:56, qinmin wrote: > > what is the max timestamp here? the new audio_timestamp_helper_ doesn't > inherit > > the max timestamp from the old one > > What do you mean? > > We preserve storage of time by transferring over the base timestamp, so > computation of max_timestamp as we pass up callback will be still be correct. > > Also, if referring to loss of frame_count information, its alright (actually, > required) since we recreate the audio track, and new track will have head > position of 0. Clarification: We don't TRANSFER base timestamp, we set new one to be current time at point of config change. ALSO: why is base_timestamp_ a member variable? We don't seem to ever actually read from it, only use it as storage (I could see how maybe, if we did use this, it could be improper to drop that information. I figured it existed to be in sync with helper however, so I kept things like this)
https://codereview.chromium.org/805273007/diff/80001/media/base/android/audio... File media/base/android/audio_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/80001/media/base/android/audio... media/base/android/audio_decoder_job.cc:85: audio_timestamp_helper_->SetBaseTimestamp(base_timestamp_); On 2015/01/21 23:00:47, kjoswiak wrote: > On 2015/01/21 22:53:11, kjoswiak wrote: > > On 2015/01/21 21:43:56, qinmin wrote: > > > what is the max timestamp here? the new audio_timestamp_helper_ doesn't > > inherit > > > the max timestamp from the old one > > > > What do you mean? > > > > We preserve storage of time by transferring over the base timestamp, so > > computation of max_timestamp as we pass up callback will be still be correct. > > > > Also, if referring to loss of frame_count information, its alright (actually, > > required) since we recreate the audio track, and new track will have head > > position of 0. > > Clarification: We don't TRANSFER base timestamp, we set new one to be current > time at point of config change. ALSO: why is base_timestamp_ a member variable? > We don't seem to ever actually read from it, only use it as storage (I could see > how maybe, if we did use this, it could be improper to drop that information. I > figured it existed to be in sync with helper however, so I kept things like > this) Suppose we got the following situation: audioTrack has buffered 10 seconds of data, and the current presentation timestep is 5 seconds, then audio_timestamp_helper_ should be (5,15). Now the next audio chunk came in, it reported a format change. Suppose 1 second has passed since the last chunk has been decoded, the current presentation timestamp should be 6. So we reset the AudioTrack and we lose all the data from 6 seconds to 15 seconds, and we create a new AudioTimeStampHelper with (6, 6). Now when we decode the next audio chunk, the audio_timestamp_helper will think that the new frame should begin at 6 second, but actually that frame is for the 15th second. And we will accumulate those errors whenever a format change happens.
On 2015/01/22 00:07:45, qinmin wrote: > https://codereview.chromium.org/805273007/diff/80001/media/base/android/audio... > File media/base/android/audio_decoder_job.cc (right): > > https://codereview.chromium.org/805273007/diff/80001/media/base/android/audio... > media/base/android/audio_decoder_job.cc:85: > audio_timestamp_helper_->SetBaseTimestamp(base_timestamp_); > On 2015/01/21 23:00:47, kjoswiak wrote: > > On 2015/01/21 22:53:11, kjoswiak wrote: > > > On 2015/01/21 21:43:56, qinmin wrote: > > > > what is the max timestamp here? the new audio_timestamp_helper_ doesn't > > > inherit > > > > the max timestamp from the old one > > > > > > What do you mean? > > > > > > We preserve storage of time by transferring over the base timestamp, so > > > computation of max_timestamp as we pass up callback will be still be > correct. > > > > > > Also, if referring to loss of frame_count information, its alright > (actually, > > > required) since we recreate the audio track, and new track will have head > > > position of 0. > > > > Clarification: We don't TRANSFER base timestamp, we set new one to be current > > time at point of config change. ALSO: why is base_timestamp_ a member > variable? > > We don't seem to ever actually read from it, only use it as storage (I could > see > > how maybe, if we did use this, it could be improper to drop that information. > I > > figured it existed to be in sync with helper however, so I kept things like > > this) > > Suppose we got the following situation: > audioTrack has buffered 10 seconds of data, and the current presentation > timestep is 5 seconds, then audio_timestamp_helper_ should be (5,15). > Now the next audio chunk came in, it reported a format change. Suppose 1 second > has passed since the last chunk has been decoded, > the current presentation timestamp should be 6. So we reset the AudioTrack and > we lose all the data from 6 seconds to 15 seconds, and we create a new > AudioTimeStampHelper with (6, 6). > Now when we decode the next audio chunk, the audio_timestamp_helper will think > that the new frame should begin at 6 second, but actually that frame is for the > 15th second. And we will accumulate those errors whenever a format change > happens. Just checking you saw these lines: if (audio_timestamp_helper_) base_timestamp_ = audio_timestamp_helper_->GetTimestamp(); AudioTimestampHelper stores a range from a base time to a current time (value returned by GetTimestamp()), and in our case the current time is the value that was the last max_presentation_time we handed to callback. We save current and make it the new base. So we would go from (5, 15) to (15,15), preserving GetTimestamp() return value. We never explicitly use the base, its preservation does not matter. Storing the frame count does matter, but see above. When we call the following line in ReleaseOutputBuffers() for, lets say, 1s of newly decoded buffers: audio_timestamp_helper_->AddFrames(size / bytes_per_frame_); We will go to (15, 16) This may still be wrong. Are we losing buffers when we destroy and remake the AudioTrack, and thus have incorrect timing on buffers added to new object, or does it pass to system queue that will outlive object? I'm thinking the former, given your concern and after reading doc on MODE_STREAM (what we are using). I'm also looking at AudioTrack API, and it does have a SetPlaybackRate() method that can change sample rate, but not sure if what we want. @gunsch Jesse, did you try SetPlaybackRate() with https://codereview.chromium.org/813643002 and found it didn't work, or was that CL just a quick fix?
On 2015/01/22 02:10:10, kjoswiak wrote: > On 2015/01/22 00:07:45, qinmin wrote: > > > https://codereview.chromium.org/805273007/diff/80001/media/base/android/audio... > > File media/base/android/audio_decoder_job.cc (right): > > > > > https://codereview.chromium.org/805273007/diff/80001/media/base/android/audio... > > media/base/android/audio_decoder_job.cc:85: > > audio_timestamp_helper_->SetBaseTimestamp(base_timestamp_); > > On 2015/01/21 23:00:47, kjoswiak wrote: > > > On 2015/01/21 22:53:11, kjoswiak wrote: > > > > On 2015/01/21 21:43:56, qinmin wrote: > > > > > what is the max timestamp here? the new audio_timestamp_helper_ doesn't > > > > inherit > > > > > the max timestamp from the old one > > > > > > > > What do you mean? > > > > > > > > We preserve storage of time by transferring over the base timestamp, so > > > > computation of max_timestamp as we pass up callback will be still be > > correct. > > > > > > > > Also, if referring to loss of frame_count information, its alright > > (actually, > > > > required) since we recreate the audio track, and new track will have head > > > > position of 0. > > > > > > Clarification: We don't TRANSFER base timestamp, we set new one to be > current > > > time at point of config change. ALSO: why is base_timestamp_ a member > > variable? > > > We don't seem to ever actually read from it, only use it as storage (I could > > see > > > how maybe, if we did use this, it could be improper to drop that > information. > > I > > > figured it existed to be in sync with helper however, so I kept things like > > > this) > > > > Suppose we got the following situation: > > audioTrack has buffered 10 seconds of data, and the current presentation > > timestep is 5 seconds, then audio_timestamp_helper_ should be (5,15). > > Now the next audio chunk came in, it reported a format change. Suppose 1 > second > > has passed since the last chunk has been decoded, > > the current presentation timestamp should be 6. So we reset the AudioTrack and > > we lose all the data from 6 seconds to 15 seconds, and we create a new > > AudioTimeStampHelper with (6, 6). > > Now when we decode the next audio chunk, the audio_timestamp_helper will think > > that the new frame should begin at 6 second, but actually that frame is for > the > > 15th second. And we will accumulate those errors whenever a format change > > happens. > > Just checking you saw these lines: > > if (audio_timestamp_helper_) > base_timestamp_ = audio_timestamp_helper_->GetTimestamp(); > > AudioTimestampHelper stores a range from a base time to a current time (value > returned by GetTimestamp()), and in our case the current time is the value that > was the last max_presentation_time we handed to callback. We save current and > make it the new base. So we would go from (5, 15) to (15,15), preserving > GetTimestamp() return value. We never explicitly use the base, its preservation > does not matter. Storing the frame count does matter, but see above. > > When we call the following line in ReleaseOutputBuffers() for, lets say, 1s of > newly decoded buffers: > > audio_timestamp_helper_->AddFrames(size / bytes_per_frame_); > > We will go to (15, 16) > > This may still be wrong. Are we losing buffers when we destroy and remake the > AudioTrack, and thus have incorrect timing on buffers added to new object, or > does it pass to system queue that will outlive object? I'm thinking the former, > given your concern and after reading doc on MODE_STREAM (what we are using). > > I'm also looking at AudioTrack API, and it does have a SetPlaybackRate() method > that can change sample rate, but not sure if what we want. > > @gunsch Jesse, did you try SetPlaybackRate() with > https://codereview.chromium.org/813643002 and found it didn't work, or was that > CL just a quick fix? Ah...Sorry, you are right. I confused AudioTimeStampHelper with TimeDeltaInterpolator. The former doesn't gave interpolated timestamps. AudioTrack.SetPlaybackRate should work as long as the AudioTrack is initialized. I am worried that tearing down an AudioTrack object and creating a new one will cause a lot of glitches, especially when there is only a small sample rate change due to some decoder errors (44100->44200 e.g).
On 2015/01/22 02:39:15, qinmin wrote: > On 2015/01/22 02:10:10, kjoswiak wrote: > > On 2015/01/22 00:07:45, qinmin wrote: > > > > > > https://codereview.chromium.org/805273007/diff/80001/media/base/android/audio... > > > File media/base/android/audio_decoder_job.cc (right): > > > > > > > > > https://codereview.chromium.org/805273007/diff/80001/media/base/android/audio... > > > media/base/android/audio_decoder_job.cc:85: > > > audio_timestamp_helper_->SetBaseTimestamp(base_timestamp_); > > > On 2015/01/21 23:00:47, kjoswiak wrote: > > > > On 2015/01/21 22:53:11, kjoswiak wrote: > > > > > On 2015/01/21 21:43:56, qinmin wrote: > > > > > > what is the max timestamp here? the new audio_timestamp_helper_ > doesn't > > > > > inherit > > > > > > the max timestamp from the old one > > > > > > > > > > What do you mean? > > > > > > > > > > We preserve storage of time by transferring over the base timestamp, so > > > > > computation of max_timestamp as we pass up callback will be still be > > > correct. > > > > > > > > > > Also, if referring to loss of frame_count information, its alright > > > (actually, > > > > > required) since we recreate the audio track, and new track will have > head > > > > > position of 0. > > > > > > > > Clarification: We don't TRANSFER base timestamp, we set new one to be > > current > > > > time at point of config change. ALSO: why is base_timestamp_ a member > > > variable? > > > > We don't seem to ever actually read from it, only use it as storage (I > could > > > see > > > > how maybe, if we did use this, it could be improper to drop that > > information. > > > I > > > > figured it existed to be in sync with helper however, so I kept things > like > > > > this) > > > > > > Suppose we got the following situation: > > > audioTrack has buffered 10 seconds of data, and the current presentation > > > timestep is 5 seconds, then audio_timestamp_helper_ should be (5,15). > > > Now the next audio chunk came in, it reported a format change. Suppose 1 > > second > > > has passed since the last chunk has been decoded, > > > the current presentation timestamp should be 6. So we reset the AudioTrack > and > > > we lose all the data from 6 seconds to 15 seconds, and we create a new > > > AudioTimeStampHelper with (6, 6). > > > Now when we decode the next audio chunk, the audio_timestamp_helper will > think > > > that the new frame should begin at 6 second, but actually that frame is for > > the > > > 15th second. And we will accumulate those errors whenever a format change > > > happens. > > > > Just checking you saw these lines: > > > > if (audio_timestamp_helper_) > > base_timestamp_ = audio_timestamp_helper_->GetTimestamp(); > > > > AudioTimestampHelper stores a range from a base time to a current time (value > > returned by GetTimestamp()), and in our case the current time is the value > that > > was the last max_presentation_time we handed to callback. We save current and > > make it the new base. So we would go from (5, 15) to (15,15), preserving > > GetTimestamp() return value. We never explicitly use the base, its > preservation > > does not matter. Storing the frame count does matter, but see above. > > > > When we call the following line in ReleaseOutputBuffers() for, lets say, 1s of > > newly decoded buffers: > > > > audio_timestamp_helper_->AddFrames(size / bytes_per_frame_); > > > > We will go to (15, 16) > > > > This may still be wrong. Are we losing buffers when we destroy and remake the > > AudioTrack, and thus have incorrect timing on buffers added to new object, or > > does it pass to system queue that will outlive object? I'm thinking the > former, > > given your concern and after reading doc on MODE_STREAM (what we are using). > > > > I'm also looking at AudioTrack API, and it does have a SetPlaybackRate() > method > > that can change sample rate, but not sure if what we want. > > > > @gunsch Jesse, did you try SetPlaybackRate() with > > https://codereview.chromium.org/813643002 and found it didn't work, or was > that > > CL just a quick fix? > > Ah...Sorry, you are right. I confused AudioTimeStampHelper with > TimeDeltaInterpolator. The former doesn't gave interpolated timestamps. > AudioTrack.SetPlaybackRate should work as long as the AudioTrack is initialized. > I am worried that tearing down an AudioTrack object and creating a new one will > cause a lot of glitches, especially when there is only a small sample rate > change due to some decoder errors (44100->44200 e.g). I tried SetPlaybackRate first, and I recall it not working, though I don't quite remember what the symptoms were. You could try it out along with this CL; it's possible that it will work correctly now that you're also updating the AudioTimestampHelper. @qinmin: for what it's worth, we haven't had reports of glitches for Cast on ATV, either in manual QA or dogfood, and have been running with the patch to reset AudioTrack for ~a month. But I agree that it's still a concern and would prefer not to if possible.
Working on a version that uses SetPlaybackRate w/o destroying old AudioTrack, but need to rework some of my fix to make this possible (the assumption that new frame count is 0 is no longer true if we are no longer dealing with a newly created track, and this causes playback to crash, sometimes). Current version of CL does otherwise seem to work, minus the sync accumulation (which isn't that bad, and certainly better than prior behavior)
Changed to use setPlaybackRate. Basically, just added a frame_count_ member to AudioDecoderJob since we can no longer rely on TimestampHelper to track frame counts. Also cleaned up base_timestamp_ member, which was never used as a proper member variable (if there is a reason it was stored as member, can revert this) Things seem to work as well as with reset. Unfortunately, that AV sync accumulation appears to still be present (I suspect it might be different bug) https://codereview.chromium.org/805273007/diff/100001/media/base/android/audi... File media/base/android/audio_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/100001/media/base/android/audi... media/base/android/audio_decoder_job.cc:58: frame_count_ = 0; This behavior was overlooked before (and I haven't seen present issue), but in theory if we are syncing with getPlaybackHeadPosition() we need to reset our count when we flush (technically flush call happens later, but should be safe to do here) Will remove if deemed unnecessary.
https://codereview.chromium.org/805273007/diff/100001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/805273007/diff/100001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:697: private boolean reconfigureAudioTrack(MediaFormat format) { Note: if you go this route (setPlaybackRate + AudioTimestampHelper instead of resetting the AudioTrack), can you also: 1) Confirm b/17476233 and b/18746104 are still okay (there were about six different bugs but these two should cover it) 2) remove "reconfigureAudioTrack" entirely? There's now only one call site (line 680). It would essentially be reverting my previous CL except for the setPlaybackRate part.
https://codereview.chromium.org/805273007/diff/100001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/805273007/diff/100001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:697: private boolean reconfigureAudioTrack(MediaFormat format) { On 2015/01/23 22:31:40, gunsch wrote: > Note: if you go this route (setPlaybackRate + AudioTimestampHelper instead of > resetting the AudioTrack), can you also: > > 1) Confirm b/17476233 and b/18746104 are still okay (there were about six > different bugs but these two should cover it) > > 2) remove "reconfigureAudioTrack" entirely? There's now only one call site (line > 680). It would essentially be reverting my previous CL except for the > setPlaybackRate part. 1) Daily Burn stream provided in b/17476233 seems to work. Specific url for SVT was dead (or at least I couldn't access), but downloaded the app and ran a few random streams and didn't see an issue. 2) Done.
ping. Anything else I should double check?
The CQ bit was checked by kjoswiak@chromium.org
The CQ bit was unchecked by kjoswiak@chromium.org
The CQ bit was checked by kjoswiak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/805273007/120001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/805273007/diff/120001/media/base/android/audi... File media/base/android/audio_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/120001/media/base/android/audi... media/base/android/audio_decoder_job.cc:82: audio_timestamp_helper_->SetBaseTimestamp(base_timestamp); if a seek is performed before audio_timestamp_helper_ is constructed, isn't we losing the base_timestamp when the audio_timestamp_helper is created later? https://codereview.chromium.org/805273007/diff/120001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/805273007/diff/120001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:577: if (mAudioTrack.setPlaybackRate(newSampleRate) != AudioTrack.SUCCESS) { Shouldn't there be a check that whether this MediaCodec object is for audio? https://codereview.chromium.org/805273007/diff/120001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/120001/media/base/android/medi... media/base/android/media_decoder_job.cc:654: } nit: "}" can be moved to the previous line https://codereview.chromium.org/805273007/diff/120001/media/base/android/medi... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/805273007/diff/120001/media/base/android/medi... media/base/android/media_decoder_job.h:237: virtual void GotOutputFormatChanged(); nit: Normally functions like this are named to "OnOutputFormatChanged()"
https://codereview.chromium.org/805273007/diff/120001/media/base/android/audi... File media/base/android/audio_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/120001/media/base/android/audi... media/base/android/audio_decoder_job.cc:82: audio_timestamp_helper_->SetBaseTimestamp(base_timestamp); On 2015/01/27 22:37:01, qinmin wrote: > if a seek is performed before audio_timestamp_helper_ is constructed, isn't we > losing the base_timestamp when the audio_timestamp_helper is created later? Ahh I see. Ok, reverted https://codereview.chromium.org/805273007/diff/120001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/805273007/diff/120001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:577: if (mAudioTrack.setPlaybackRate(newSampleRate) != AudioTrack.SUCCESS) { On 2015/01/27 22:37:01, qinmin wrote: > Shouldn't there be a check that whether this MediaCodec object is for audio? See mAudioTrack != null in containing if statement (unless this is not sufficient?). https://codereview.chromium.org/805273007/diff/120001/media/base/android/medi... File media/base/android/media_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/120001/media/base/android/medi... media/base/android/media_decoder_job.cc:654: } On 2015/01/27 22:37:01, qinmin wrote: > nit: "}" can be moved to the previous line Done. https://codereview.chromium.org/805273007/diff/120001/media/base/android/medi... File media/base/android/media_decoder_job.h (right): https://codereview.chromium.org/805273007/diff/120001/media/base/android/medi... media/base/android/media_decoder_job.h:237: virtual void GotOutputFormatChanged(); On 2015/01/27 22:37:01, qinmin wrote: > nit: Normally functions like this are named to "OnOutputFormatChanged()" Done.
lgtm % nit https://codereview.chromium.org/805273007/diff/140001/media/base/android/audi... File media/base/android/audio_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/140001/media/base/android/audi... media/base/android/audio_decoder_job.cc:167: if (!media_codec_bridge_) nit: from media_decoder_job.cc, this should never happen. Either change this to an assert or remove this.
https://codereview.chromium.org/805273007/diff/140001/media/base/android/audi... File media/base/android/audio_decoder_job.cc (right): https://codereview.chromium.org/805273007/diff/140001/media/base/android/audi... media/base/android/audio_decoder_job.cc:167: if (!media_codec_bridge_) On 2015/01/27 23:31:41, qinmin wrote: > nit: from media_decoder_job.cc, this should never happen. Either change this to > an assert or remove this. I see, cause it originates from bridge. Made it a DCHECK, just to be safe.
The CQ bit was checked by kjoswiak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/805273007/180001
lgtm
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/aa9cc0db413c376590cf974b52b847a46dd14c77 Cr-Commit-Position: refs/heads/master@{#313416} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
