New issue
Advanced search Search tips

Issue 868088 link

Starred by 3 users
Status: Fixed
Owner:
yuweih@chromium.org
Closed: Aug 20
Cc:
yuweih@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

[CRD iOS] Reworking the audio playback logic

Project Member Reported by yuweih@chromium.org, Jul 26 
The currently audio playback implementation has several issues:

* An audio player playing on the background will get muted by the app when an audio packet comes.
* Only support 48000kHz sample rate.
* Not properly handling playback failure case, which causes quite a few crashes when the app resumes from background.
* A few bugs untested in unittests. E.g. next_frame here doesn't account for bytes_extracted_:
  https://cs.chromium.org/chromium/src/remoting/client/audio/audio_player_buffer.cc?l=160&rcl=0c006dc4fc5bcfb225ddd7e2528944f57d6e842a
* No unittests for the native code.
* Messy thread managing. E.g. AudioPlayerIos is used on the audio thread but it needs to be deleted on a network thread.
* The code is hard to follow. Some logic-class breakdown doesn't make much sense.
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 26 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3964dbae965aa9bcc02c78aaa3fbbca98d8d51ec

commit 3964dbae965aa9bcc02c78aaa3fbbca98d8d51ec
Author: Yuwei Huang <yuweih@chromium.org>
Date: Thu Jul 26 23:05:22 2018

[CRD iOS] Allow host audio to mix with sounds from other apps

Some users complain about background audio player being muted by the app.
This CL overrides some settings in the AudioSession to allow audio from
the app to be mixed with sounds from other apps.

Bug:  868088 
Change-Id: I10af925e3eb488498ab83df25cbd77cea8ebd186
Reviewed-on: https://chromium-review.googlesource.com/1152182
Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578472}
[modify] https://crrev.com/3964dbae965aa9bcc02c78aaa3fbbca98d8d51ec/remoting/ios/app/app_delegate.mm
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 3 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5c93d5cc6841a22fd7eb982dd9393e883dd76a0d

commit 5c93d5cc6841a22fd7eb982dd9393e883dd76a0d
Author: Yuwei Huang <yuweih@chromium.org>
Date: Fri Aug 03 23:58:18 2018

[Remoting Mobile] Implementing AudioJitterBuffer

This is part of the work to rewire the audio playback logic for iOS
(and potentially also for Android).

AudioJitterBuffer acts as a jitter buffer that queues up AudioPackets
once they are received and before they are consumed by a native playback
queue.
This class is basically modified from AudioPlayerBuffer with a few
changes:
* Allow handling packets in different stream formats/sample rate and
  call a callback when the format is changed.
* Move the underrun protection logic from AudioPlayerIos (the
  "PRIMING" state) into the jitter buffer.
* Fix a bug where bytes_extracted_ is ignored when copying audio data.
* Removed the AudioStreamConsumer inheritance, Stop() and other unused
  logic.

Bug:  868088 
Change-Id: Ibaee655df28de68f5303d735ef89cf15695ab2ff
Reviewed-on: https://chromium-review.googlesource.com/1150936
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580709}
[modify] https://crrev.com/5c93d5cc6841a22fd7eb982dd9393e883dd76a0d/remoting/client/audio/BUILD.gn
[add] https://crrev.com/5c93d5cc6841a22fd7eb982dd9393e883dd76a0d/remoting/client/audio/async_audio_data_supplier.cc
[add] https://crrev.com/5c93d5cc6841a22fd7eb982dd9393e883dd76a0d/remoting/client/audio/async_audio_data_supplier.h
[add] https://crrev.com/5c93d5cc6841a22fd7eb982dd9393e883dd76a0d/remoting/client/audio/audio_jitter_buffer.cc
[add] https://crrev.com/5c93d5cc6841a22fd7eb982dd9393e883dd76a0d/remoting/client/audio/audio_jitter_buffer.h
[add] https://crrev.com/5c93d5cc6841a22fd7eb982dd9393e883dd76a0d/remoting/client/audio/audio_jitter_buffer_unittest.cc
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 9 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40c532242068095ad57b3d86a1789e83e02d58c6

commit 40c532242068095ad57b3d86a1789e83e02d58c6
Author: Yuwei Huang <yuweih@chromium.org>
Date: Thu Aug 09 05:42:27 2018

[CRD iOS] Implement AudioPlaybackSinkIos

This is part of the work of refactoring the iOS audio playback logic.

This CL implements the AudioPlaybackSink for iOS using the AudioQueue
API. The implementation is following Apple's guide for the playback
process:
https://developer.apple.com/library/archive/documentation/MusicAudio/Conceptual/AudioQueueProgrammingGuide/AboutAudioQueues/AboutAudioQueues.html#//apple_ref/doc/uid/TP40005343-CH5-SW22

In the next CL there will be an AudioPlaybackStream class that
connects the AudioJitterBuffer with AudioPlaybackSinkIos.

Bug:  868088 
Change-Id: Ifbaa08bfcf5f983587bfb71418c681001ab7cdca
Reviewed-on: https://chromium-review.googlesource.com/1166246
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581800}
[modify] https://crrev.com/40c532242068095ad57b3d86a1789e83e02d58c6/remoting/client/audio/BUILD.gn
[modify] https://crrev.com/40c532242068095ad57b3d86a1789e83e02d58c6/remoting/client/audio/async_audio_data_supplier.h
[modify] https://crrev.com/40c532242068095ad57b3d86a1789e83e02d58c6/remoting/client/audio/audio_jitter_buffer.cc
[modify] https://crrev.com/40c532242068095ad57b3d86a1789e83e02d58c6/remoting/client/audio/audio_jitter_buffer.h
[modify] https://crrev.com/40c532242068095ad57b3d86a1789e83e02d58c6/remoting/client/audio/audio_jitter_buffer_unittest.cc
[add] https://crrev.com/40c532242068095ad57b3d86a1789e83e02d58c6/remoting/client/audio/audio_playback_sink.h
[add] https://crrev.com/40c532242068095ad57b3d86a1789e83e02d58c6/remoting/client/audio/audio_stream_format.cc
[add] https://crrev.com/40c532242068095ad57b3d86a1789e83e02d58c6/remoting/client/audio/audio_stream_format.h
[add] https://crrev.com/40c532242068095ad57b3d86a1789e83e02d58c6/remoting/client/audio/fake_async_audio_data_supplier.cc
[add] https://crrev.com/40c532242068095ad57b3d86a1789e83e02d58c6/remoting/client/audio/fake_async_audio_data_supplier.h
[modify] https://crrev.com/40c532242068095ad57b3d86a1789e83e02d58c6/remoting/ios/BUILD.gn
[modify] https://crrev.com/40c532242068095ad57b3d86a1789e83e02d58c6/remoting/ios/audio/BUILD.gn
[add] https://crrev.com/40c532242068095ad57b3d86a1789e83e02d58c6/remoting/ios/audio/audio_playback_sink_ios.cc
[add] https://crrev.com/40c532242068095ad57b3d86a1789e83e02d58c6/remoting/ios/audio/audio_playback_sink_ios.h
[add] https://crrev.com/40c532242068095ad57b3d86a1789e83e02d58c6/remoting/ios/audio/audio_playback_sink_ios_unittest.cc

Comment 4 by yuweih@chromium.org, Aug 10

Cc: yuweih@chromium.org
 Issue 838744  has been merged into this issue.

Comment 5 by yuweih@chromium.org, Aug 10 

 Issue 828203  has been merged into this issue.
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 10 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b1e640c47e6cb75a8385ebba2e2aa31aeba732bf

commit b1e640c47e6cb75a8385ebba2e2aa31aeba732bf
Author: Yuwei Huang <yuweih@chromium.org>
Date: Fri Aug 10 22:43:50 2018

[CRD iOS] Using the new audio playback logic

This CL finishes up the rest of the work of refactoring the audio
playback logic on iOS.

Bug:  868088 
Change-Id: Iac4c240a8f98a1212bf65c056546f2f3ad14dd2d
Reviewed-on: https://chromium-review.googlesource.com/1166339
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582365}
[modify] https://crrev.com/b1e640c47e6cb75a8385ebba2e2aa31aeba732bf/remoting/client/audio/BUILD.gn
[add] https://crrev.com/b1e640c47e6cb75a8385ebba2e2aa31aeba732bf/remoting/client/audio/audio_playback_stream.cc
[add] https://crrev.com/b1e640c47e6cb75a8385ebba2e2aa31aeba732bf/remoting/client/audio/audio_playback_stream.h
[modify] https://crrev.com/b1e640c47e6cb75a8385ebba2e2aa31aeba732bf/remoting/ios/session/remoting_client.mm
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 11 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/206d44a2086dc771c898b5d3f9da0bcd95f6c0a7

commit 206d44a2086dc771c898b5d3f9da0bcd95f6c0a7
Author: Yuwei Huang <yuweih@chromium.org>
Date: Sat Aug 11 04:59:12 2018

[CRD iOS] Make ChromotingSession own the AudioStub

Previously AudioStub is owned by the the (Remoting|Jni)Client class and
being passed to ChromotingSession as a WeakPtr. The AudioStub is not
used after it is passed to ChromotingSession and it is a burden for the
*Client class to manage its lifetime.

This CL fixes this by making ChromotingSession own the AudioStub.

Bug:  868088 
Change-Id: I067f321b3aab7110df7bb85a8b8c519ea889865e
Reviewed-on: https://chromium-review.googlesource.com/1171840
Reviewed-by: Joe Downing <joedow@chromium.org>
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582428}
[modify] https://crrev.com/206d44a2086dc771c898b5d3f9da0bcd95f6c0a7/remoting/client/audio/audio_playback_stream.cc
[modify] https://crrev.com/206d44a2086dc771c898b5d3f9da0bcd95f6c0a7/remoting/client/audio/audio_playback_stream.h
[modify] https://crrev.com/206d44a2086dc771c898b5d3f9da0bcd95f6c0a7/remoting/client/chromoting_session.cc
[modify] https://crrev.com/206d44a2086dc771c898b5d3f9da0bcd95f6c0a7/remoting/client/chromoting_session.h
[modify] https://crrev.com/206d44a2086dc771c898b5d3f9da0bcd95f6c0a7/remoting/client/jni/jni_client.cc
[modify] https://crrev.com/206d44a2086dc771c898b5d3f9da0bcd95f6c0a7/remoting/client/jni/jni_client.h
[modify] https://crrev.com/206d44a2086dc771c898b5d3f9da0bcd95f6c0a7/remoting/ios/session/remoting_client.mm
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 15 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5619f416cfcd3358bae19e81c3be86c83a1a1bfa

commit 5619f416cfcd3358bae19e81c3be86c83a1a1bfa
Author: Yuwei Huang <yuweih@chromium.org>
Date: Wed Aug 15 18:43:36 2018

[CRD iOS] Remove old audio playback code

This CL removes old audio playback code that is no longer in used.

Bug:  868088 
Change-Id: I0dc350fc6d7fcabe89c6c7deba1da34fe53c3667
Reviewed-on: https://chromium-review.googlesource.com/1171855
Reviewed-by: Joe Downing <joedow@chromium.org>
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583340}
[modify] https://crrev.com/5619f416cfcd3358bae19e81c3be86c83a1a1bfa/remoting/client/audio/BUILD.gn
[delete] https://crrev.com/c65043058548656ebcae516efae3359fc82c627b/remoting/client/audio/async_audio_frame_supplier.h
[delete] https://crrev.com/c65043058548656ebcae516efae3359fc82c627b/remoting/client/audio/audio_frame_supplier.h
[delete] https://crrev.com/c65043058548656ebcae516efae3359fc82c627b/remoting/client/audio/audio_player_buffer.cc
[delete] https://crrev.com/c65043058548656ebcae516efae3359fc82c627b/remoting/client/audio/audio_player_buffer.h
[delete] https://crrev.com/c65043058548656ebcae516efae3359fc82c627b/remoting/client/audio/audio_player_buffer_unittest.cc
[delete] https://crrev.com/c65043058548656ebcae516efae3359fc82c627b/remoting/client/audio/audio_stream_consumer.cc
[delete] https://crrev.com/c65043058548656ebcae516efae3359fc82c627b/remoting/client/audio/audio_stream_consumer.h
[modify] https://crrev.com/5619f416cfcd3358bae19e81c3be86c83a1a1bfa/remoting/ios/audio/BUILD.gn
[delete] https://crrev.com/c65043058548656ebcae516efae3359fc82c627b/remoting/ios/audio/audio_player_ios.h
[delete] https://crrev.com/c65043058548656ebcae516efae3359fc82c627b/remoting/ios/audio/audio_player_ios.mm
[delete] https://crrev.com/c65043058548656ebcae516efae3359fc82c627b/remoting/ios/audio/audio_stream_consumer_proxy.cc
[delete] https://crrev.com/c65043058548656ebcae516efae3359fc82c627b/remoting/ios/audio/audio_stream_consumer_proxy.h

Comment 9 by yuweih@chromium.org, Aug 20

Status: Fixed (was: Assigned)
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 30 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc36da272240d268ab956b5d260359fc7497b2b1

commit cc36da272240d268ab956b5d260359fc7497b2b1
Author: Yuwei Huang <yuweih@chromium.org>
Date: Thu Aug 30 01:40:04 2018

[CRD iOS] Change audio session category to Ambient

AVAudioSessionCategoryAmbient works basically like
AVAudioSessionCategoryPlayback+AVAudioSessionCategoryOptionMixWithOthers,
except that it allows the audio to be muted by the silent switch.
This a more proper category to use because the app can still work with no
sound. Only apps like music player that requires audio playback when
silent switch is on should use the AVAudioSessionCategoryPlayback
category.

Bug:  868088 
Change-Id: I56163a221d878f67363aa8cff7a0151e9602e1c1
Reviewed-on: https://chromium-review.googlesource.com/1196020
Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587393}
[modify] https://crrev.com/cc36da272240d268ab956b5d260359fc7497b2b1/remoting/ios/app/app_delegate.mm
►

Sign in to add a comment