-
Notifications
You must be signed in to change notification settings - Fork 103
iOS 向けに AudioTrackSink を追加する #128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
!haiku レビューして |
This comment was marked as resolved.
This comment was marked as resolved.
|
!haiku レビューして |
This comment was marked as resolved.
This comment was marked as resolved.
|
!haiku 以下の観点でレビューして
|
This comment was marked as resolved.
This comment was marked as resolved.
| steps: | ||
| - uses: actions/checkout@v5 | ||
| - name: Select Xcode 16.1 | ||
| - name: Select Xcode 26.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修正が漏れていたのでついでに対応しました
patches/ios_audio_track_sink.patch
Outdated
| + | ||
| +/** | ||
| + * このアダプターが公開するネイティブの AudioTrackSinkInterface。呼び出しは生成時に | ||
| + * 渡された RTCAudioTrackSink へ変換されます。このポインタは非安全で、このクラスが所有します。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「呼び出しは生成時に渡された RTCAudioTrackSink へ変換されます。」の意味が全く理解できないので、もうちょっとコメント考えてほしいです。
「このポインタは非安全で、このクラスが所有します。」も、非安全という書き方もあまり見かけないし、非安全がどういう意味なのかが分からないです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
実態は AudioTrackSinkAdapter を見ればわかると思ったので、ここもコメント削除します
patches/ios_audio_track_sink.patch
Outdated
| + * webrtc::AudioTrackSinkInterface は WebRTC の音声処理で使われ、このアダプターが | ||
| + * その呼び出しを生成時に渡された RTCAudioTrackSink へ橋渡しします。 | ||
| + */ | ||
| +@interface RTCAudioTrackSinkAdapter : NSObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このクラスは公開する必要なさそうなので、このファイルは不要そうに見えます
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
削除しました
patches/ios_audio_track_sink.patch
Outdated
| - (instancetype)init NS_UNAVAILABLE; | ||
|
|
||
| -/** The audio source for this audio track. */ | ||
| +/** このオーディオトラックの音声ソース。 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここは変更する必要なさそう
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
戻しました
patches/ios_audio_track_sink.md
Outdated
| } | ||
|
|
||
| func preferredNumberOfChannels() -> Int { | ||
| return 1 // 例: モノラルで受けたい場合。指定しなければ -1 を返す。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
指定しなければ -1 を返す
「実装しなければ -1 を返すデフォルト実装が使われる」
「特にチャンネル数を指定しない場合は -1 を返しても構わない」
あたりかな。ただ後者の書き方だとこの関数の実装をサボれることが分からないので、前者寄りの書き方が良さそうな気がします。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます。前者の書き方に修正します。
patches/ios_audio_track_sink.md
Outdated
|
|
||
| ### パッチ実装のポイント | ||
|
|
||
| - `RTCAudioTrackSinkAdapter` は `AudioTrackSinkInterface` を実装し、`OnData` で受け取った PCM を `NSData` にコピーして `RTCAudioTrackSink#onData` に渡す。コールバックはネイティブ音声スレッドで呼ばれるため、重い処理は別スレッドへ委譲する必要があることに注意。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RTCAudioTrackSinkAdapterはAudioTrackSinkInterfaceを実装し
AudioTrackSinkInterface を実装しているのは AudioTrackSinkAdapter のはず。
RTCAudioTrackSink#onDataに渡す
# で繋ぐのは Java の書き方なので、Objective-C 的には「RTCAudioTrackSink の onData:bitsPerSample:sampleRate:numberOfChannels:numberOfFrames: に渡す」かな。
別スレッドへ委譲する必要がある、に関してはパッチ実装のポイントではないので不要そう。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
別スレッドへ〜の説明はドキュメント下の注意点の説明の部分に移動させました。
patches/ios_audio_track_sink.patch
Outdated
| + | ||
| +/** | ||
| + * このアダプター生成時に渡された Objective-C の音声トラックシンク。 | ||
| + * webrtc::AudioTrackSinkInterface への呼び出しはここへ変換されて渡されます。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この行のコメント理解するのにめっちゃ時間掛かったので、直せそうなら直して欲しいかも。
そもそもこの説明を書く必要あるかが疑問。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
header file の方で使われ方について実装も交えたコメントをわざわざ書くのもおかしいと思ったので、説明を削除しました
| + return static_cast<int>([sink preferredNumberOfChannels]); | ||
| + } | ||
| + return -1; | ||
| + } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なんかインデントおかしい気がする
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修正しました。(上の int Num ~ の先頭に合わせました
Co-authored-by: melpon <[email protected]>
Co-authored-by: melpon <[email protected]>
| + if ([sink respondsToSelector:@selector(preferredNumberOfChannels)]) { | ||
| + return static_cast<int>([sink preferredNumberOfChannels]); | ||
| + } | ||
| + return -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
インデントが2スペースであることを考えると、このあたりのインデントも直す必要がありそう。
というより、depot_tools にパスを通してから git cl format しても良いと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PATH を通したあとに、
python3 run.py revert --patch ios_audio_track_sink.patch
cd _source/ios_sdk/webrtc/src
# 新しいファイルは一度ステージングする
# git add sdk/objc/api/RTCAudioTrackSinkAdapter.mm
git cl format [file 指定]
# ステージング解除
# git restore --staged sdk/objc/api/RTCAudioTrackSinkAdapter.mm
python3 run.py diff ios_sdk > patches/ios_audio_track_sink.patchこの手順でフォーマットをかけて、パッチを更新してみます。
単純に git cl format を実行すると全く関係のないファイルまでフォーマットされたので、とりあえず必要なファイルにだけフォーマットをあてるスタイルです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9816cba で git cl format しました。
Co-authored-by: melpon <[email protected]>
Co-authored-by: melpon <[email protected]>
melpon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
よさそう
No description provided.