-
Notifications
You must be signed in to change notification settings - Fork 164
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
Allow web devs to synchronize branches with tee()? #1157
Comments
It's starting to sound like readable streams may not be a good fit for this use case... In the streams model, it is not acceptable to drop chunks in the middle of the stream. From the FAQ:
If I understand the draft spec correctly, the const [branch1, branch2] = readable.tee();
let chunks1 = [];
let chunks2 = [];
for await (let chunk of branch1) chunks1.push(chunk);
for await (let chunk of branch2) chunks2.push(chunk);
chunks1.forEach((chunk, i) => chunk === chunks2[i]); Looks like you're running into much of the same issues as WebCodecs did. 😕 |
Agreed, and this would remain true, because I'm not suggesting I don't believe there's any requirement on transforms to produce the same number of chunks they consume. E.g. Apart from
Where does that document mention this issue? |
Ah, I think I understand. Correct me if I'm wrong though! Currently, With For example: const [branch1, branch2] = readable.tee({ synchronized: true });
const bufferedBranch1 = branch1.pipeThrough(new TransformStream({}, { highWaterMark: 3 }));
const bufferedBranch2 = branch2.pipeThrough(new TransformStream({}, { highWaterMark: 5 })); With a regular This sounds pretty feasible to do in author code. I'll see if I can whip up something to experiment with, and see if that solves your problem. 👨🔬
That was assuming that you wanted to drop frames mid-stream, which was a misunderstanding on my part. |
All right, here's my attempt: https://jsfiddle.net/MattiasBuelens/9gr3zq7b/7/ It's basically a re-implementation of ReadableStreamDefaultTee, but with a modified pull algorithm that waits until both branches have called Here's how that would work in your initial example: https://jsfiddle.net/MattiasBuelens/y439ahnp/2/ |
I think we want { synchronized: true } to be the default for a ReadableStream from MediaStreamTrackProcessor. |
{ synchronized: true} would slow down the fastest consumer which is undesirable in a realtime video pipeline. If we look at what happens in existing native pipelines like 'render local + encode local', frames are being dropped as needed by both renderer and encoder.
I believe this is what we actually want in our use case.
I do not see how that would work without custom resynchronisation between the two consumers. |
It's desirable in the first use case I mentioned in the OP, and shouldn't get in the way of dropping frames downstream.
Exactly. Dropping frames downstream ("renderer and encoder") is a common strategy. A sink resolves promises at its discretion, so the streams spec isn't in the way there. What we're talking about here is how things degrade when that fails, and giving applications tools to control this. Drift (
I don't follow this characterization. If a consumer isn't ready for a frame, then the "freshest" frame is the one it has to wait for, since the one available immediately is no longer fresh. As a baseline, in the sunny case, where both branches are within the time budget of the source frame rate, both branches wait for the next (fresh) frame (i.e. in sync), and this is entirely normal and not "slow". |
As I understand it, the first use case is this one:
If it is something equivalent to salsify, the idea is to get the output of both versions and pick one. In that case, the application will synchronise the results of the two operations, synchronized=false and synchronized=true will provide the same results. The idea behind tee is that we have different consumers processing samples at their own pace.
In both synchronized=true and synchronized=false, the solution to get to the optimal is for the 'slow' consumer to read at the 'fast' consumer pace, and drop frames as needed. The question is then why we should introduce a new option if the actual solution is the same no matter whether option is set to true or false. Also this solution introduces coupling between the consumers which is something we do not want.
In that case, synchronized=true or synchronized=false have the same result. Say we use a renderer and an encoder and are using tee as these are two different consumers. Please also look at the downsides of introducing a new option in terms of complexity/learning curve/debuggability. |
I would find it quite surprising if the behavior or It would also be awkward to implement. We'd have to add an internal flag on the
Indeed. At that point, I'd say that Streams is not the right API for your use case. 😕 Perhaps you should consider building your own API to process/transform a stream of video frames, with support for synchronization and frame dropping? You could still add APIs that interoperate with Streams "on the border", e.g. a method that converts the output of a |
One possible solution is to fail usage of raw tee() with these streams. I see from the discussion of introducing tee() that it was pretty controversial when it was introduced, precisely because it makes it very easy to make patterns of buffering that are hard to reason cleanly about, and some of those patterns are footguns. (In particular, when I experimented with it, I found that tee() does not respect desiredSize on the destination stream; it enqueues stuff no matter what desiredSize is. (Unless I messed up my code, of course.) It's entirely feasible to write your own tee-like function that does exactly the queueing strategy that's desired, including dropping when appropriate - the standard tee operator + realtime streams + random jank seems like a Really Bad Combination. |
Streams that drop frames would, of course, be TransformStreams. |
Streams before tee() was introduced: fits our use case. Maybe the problem is tee() and not streams? |
This would break point 3 of where readable stream fits best, according https://github.com/whatwg/streams/blob/main/FAQ.md:
Points 2 and 3 are not met in the context of a realtime video processing pipeline. |
I have added a proposed new section to the mediacapture-transform spec (a Google proposal): |
The FAQ is trying to be helpful and reflect the (current) spec, but is not authoritative I don't think. If we improve the spec's "fit" for real-time streams (through issues like this one, #1156 and #1158), we should update the FAQ. FWIW the spec's Introduction says "the Streams Standard enables use cases like: • Video effects: piping a readable video stream through a transform stream that applies effects in real time."
As a criteria of exclusion, this seems inaccurate, since consumers are free to drop data: numbers
.pipeThrough(new TransformStream({transform: (chunk, ctrl) => chunk % 2 || ctrl.enqueue(chunk)}))
.pipeTo(new WritableStream({write: c => console.log(c)})); // 0, 2, 4, 6, 8, 10 I also see no prohibition on sources (whether a camera or a WebTransport datagram receiver) dropping data as a solution to receiving back pressure.
I think this fits, modulo this issue. Most use cases will be single consumer; a few might be multi-consumer, like this cropping example with both self-view and video "sending" that works in Chrome (with "Experimental Web Platform features" enabled).
I doubt camera source errors are recoverable. Do you mean WebCodecs errors? While writing the above fiddle, the error I ran into most was "frame closed", due to |
I have this working in Chrome now. Self-view is 30 fps for me which is all my camera can muster, and encoder is 15 fps from induced delay, but the self-view stays smooth, even when the encoder struggles. Dropping frames in a TransformStream turned out to be trivial: function FrameDropper() {
return new TransformStream({
transform(frame, controller) {
if (controller.desiredSize < 2) {
frame.close();
} else {
controller.enqueue(frame);
}
}
}, {highWaterMark: 1}, {highWaterMark: 2});
} I couldn't use I also use @MattiasBuelens's |
MediaStreamTrackProcessor exposes a real-time
ReadableStream
ofVideoFrame
s, e.g. from a camera. It'll drop frames on consumers who don't keep up, to keep data current, and also because it may have a finite pool of GPU-backed frames to vend.This seems to work well, provided consumers leave
highWaterMark
at1
.But with
tee()
, if one or both branches lag they'll drift from each other, causing one branch to stop being real-time. This causes buffer bloat buildup, eating up a ton ofVideoFrame
s.JS may work around it by counting frames and trying to synchronize the branches by making them take the same amount of time (by waiting for the slower branch), but this can be tricky to get right (spot the bug).
A
tee({synchronized: true})
option that did this for you might be better.This would help ensure both branches stay real-time, at the cost of both dropping frames if one of them lags.
This cost might seem undesirable, so let's look at two potential use cases for
tee()
:{synchronized: true}
should solve the first use case out of the box, since the ultimate sink expects both encodings.The second use case proves trickier, since we don't want to drop frames in the self-view. To do this without buffer-buildup would probably necessitate a frame-dropper transform on the branch destined for WebTransport, to get it down to 15 fps. This would be a transform that consumes higher fps, resolving promises prematurely to do so. There may also be better ways to solve the second use case without
tee()
, like going through a second MediaStreamTrack.But we can't prevent someone from using
tee()
to try to solve the second use case, so we need to consider it. But importantly, a frame-dropper transform seems like it should be able to coexist well with atee({synchronized: true})
option.Thoughts?
The text was updated successfully, but these errors were encountered: