Skip to content
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

time.time() となっていたのを time.monotonic() に修正 #102

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

tnoho
Copy link
Contributor

@tnoho tnoho commented Oct 13, 2024

説明文の修正だけなのですが、割とひどい思い込みから説明文が誤っていたので修正しました。

機械学習などで映像や音声に前処理を行っても映像と音声の同期が取れるように Python 側で timestamp を入れられるように作ってあったのですが、説明通り time.time() を入れると大幅なディレイが生じるという問題がありました。

これが思い込みからのミスで libwebrtc 内の rtc::TimeMicros(), rtc::TimeMillis() の戻り値をエポックタイムと思い込んでいました。実際にはシステム起動からの時間であるモノトニッククロックでした。
また、ビデオフレームやオーディオデータのタイムスタンプは下記の通りモノトニッククロックとする必要があります。
https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/api/media_stream_interface.h;l=214

rtc::TimeMicros() や rtc::TimeMillis() の API を作ることも考えたのですが、 Python の time.monotonic() で同じ値が取れることがわかったので、 time.time() ではなく time.monotonic() を使うように説明文を修正しました。
この修正を行うことで、ディレイも生じなくなることを確認してあります。

@tnoho tnoho requested a review from voluntas October 13, 2024 08:39
@tnoho tnoho self-assigned this Oct 13, 2024
@sile
Copy link
Member

sile commented Oct 13, 2024

https://docs.python.org/ja/3/library/time.html#time.monotonic には 戻り値の基準点は定義されていないので、二回の呼び出しの結果の差だけが有効です と記載されているので monotonic だと将来的に(or 環境によって)いつの間にか壊れている、というリスクがありそうな気がしました。

@tnoho
Copy link
Contributor Author

tnoho commented Oct 13, 2024

うーんなるほど。
そうすると rtc::TimeMicros() のラッパーを作りますか、 sora_sdk.TimeNow() みたいな名前にして。
戻り値は time.time() や time.monotonic() と同じで秒単位の double を返すような感じで。

Video は us, Audio は ms でして。us があればいいかなと言うのと、double にしておかないと破壊的変更になってしまうという問題がありまして。。。

ああでも、 Video のほうは double 以外にも us で入れることを想定した int を引数にしたフレーム送信関数もあるのでした。
そしたらこちら向けには rtc::TimeMicros() をそのままラップした sora_sdk.TimeMicros() という int を返す関数も作る。という感じでいかがでしょう。

とかなんとか書きつつ、一番いいのは破壊的変更で rtc::TimeMicros(), rtc::TimeMillis() のラップで sora_sdk.TimeMicros(). sora_sdk.TimeMillis() を作り、double 型になっているフレームやデータ入力関数を無くして int に統一するのが良いと思ってしまうのですが。

4案箇条書きにしておきます

  1. sora_sdk.TimeNow() を作り、戻り値は time.time() や time.monotonic() と同じで秒単位の double を返す
  2. 1 に加え rtc::TimeMicros() をそのままラップした sora_sdk.TimeMicros() という int を返す関数も作る
  3. 1 に加え double に統一すべくマイクロ秒で入れることを想定した int を引数にした SoraVideoSource::OnCaptured を無くす
  4. sora_sdk.TimeMicros(). sora_sdk.TimeMillis() を作り、double 型になっているフレームやデータ入力関数を無くし SoraAudioSource::OnData には int でミリ秒を SoraVideoSource::OnCaptured は int でマイクロ秒を入れるようにする

悩ましい。。。それかいっそ sora_sdk.Timestamp 型を作るべきなのか。。。(libwebrtc にはミリ秒、マイクロ秒などの変換を容易にする webrtc::Timestamp 型がある

@sile
Copy link
Member

sile commented Oct 13, 2024

確認ありがとうございます。
ここでのタイムスタンプの扱いには注意が必要、ということを利用者に示す意味でも専用の型を用意するのは賛成です。
その具体的なアプローチ(設計)には自分は拘りは薄いので @voluntas の意見をもらえると助かります。

@voluntas voluntas removed their request for review October 14, 2024 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants