Conversation
hkmt-mmy
reviewed
Oct 14, 2025
| { | ||
| try | ||
| { | ||
| try |
There was a problem hiding this comment.
ネストが深いのが気になりました。内側のtry-finallyは関数に切り出してもいいかも?
hkmt-mmy
reviewed
Oct 14, 2025
| _numChannelsInOption = numChannelsInOption; | ||
| } | ||
|
|
||
| public bool Transform(InputAudioFrame input, out PcmAudioFrame output) |
There was a problem hiding this comment.
かなり長いので (もし良い感じの分割単位があれば) 分割したほうが見通しが良さそうだと思いました
hkmt-mmy
reviewed
Oct 14, 2025
|
|
||
| public void Push(EncodedFrame value) | ||
| { | ||
| if (!_buffer.TryAddAudioFrame(value)) |
hkmt-mmy
reviewed
Oct 14, 2025
| { | ||
| Unregister(); | ||
|
|
||
| if (_disposeProvider) |
There was a problem hiding this comment.
こことかUnregisterのところも{}ついてるけど、基本的に付けないルールでやってそうなので気になりました
hkmt-mmy
reviewed
Oct 14, 2025
| public void Dispose() | ||
| { | ||
| if (_array != null) | ||
| ArrayPool<short>.Shared.Return(_array); |
There was a problem hiding this comment.
借りた当人じゃないのにReturnするのが少し気になりました。コンストラクタ引数でrendとしてるしいいのかな……?
Collaborator
Author
There was a problem hiding this comment.
この配列はタスク間を非同期的にたらい回されたあとに解放されるので、残念ながらこれについては解決策がないです
hkmt-mmy
reviewed
Oct 14, 2025
|
|
||
| private async Task TransferAsync(IAsyncPipelineInput<EncodedFrame> next) | ||
| { | ||
| try |
There was a problem hiding this comment.
ネストがめっちゃ深い……。処理自体は少ないけど、private関数なりローカル関数なりに切り出したほうが良さそう
hkmt-mmy
reviewed
Oct 14, 2025
| private readonly IRecordingTimeProvider _recordingTimeProvider; | ||
| private bool _disposed; | ||
| private double _prevFrameTime; | ||
| private readonly double? _fixedFrameInterval; |
There was a problem hiding this comment.
readonlyがついててコンストラクタ初期化されるフィールドがまとまってないのが気になりました。好みの問題かもですが
qua-iy
approved these changes
Oct 14, 2025
hkmt-mmy
approved these changes
Oct 15, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New abstraction layer of encoding pipeline to implement pipelines such as #39 and #40, which are slightly different from current one.
Concept
With the pipeline abstraction, a pipeline is represented as a sequence of pipeline elements which implement
IAsyncPipelineInput<T>orIPipelineInput<T>. This enhances the reusability of the code implementing the pipeline.IAsyncPipelineInput<T>Represents single element of encoding pipeline, which is mainly for data flow where back pressure occurs.
IPipelineInput<T>will receive values, manipulate them and pass them anotherIPipelineInput<T>.IPipelineInput<T>A synchronous version of
IAsyncPipelineInput<T>, which is mainly for data flow without back pressure.IPipelineTransform<TIn, TOut>Some pipeline elements may be able to be used in both asynchronous and synchronous flow.
IPipelineTransformonly requires simple value transform implementation and they can be converted to eitherIAsyncPipelineInput<T>orIPipelineInput<T>usingPipelineExtensions.AsInput()andPipelineExtensions.AsAsyncInput().Redesigning
RealtimeInstantReplaySessionRealtimeInstantReplaySessionis redesigned with the pipeline abstraction. The pipeline is wrote like:It has already been confirmed that General Unbounded Recording (#40) can be implemented with a similarly small amount of code with the pipeline abstraction. (PR).
Future works
Currently all pipeline abstraction APIs are private but I want to publish them, enabling users to create their own pipelines easily.