-
Notifications
You must be signed in to change notification settings - Fork 130
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
Add detailed design discussion for codec configuration #14
Conversation
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.
I like this a lot. Would it make sense to also update the webidl.txt file?
090f931
to
c878bea
Compare
I took a stab at updating the webidl.txt. To be honest, I wonder if trying to map everything to webidl might be more confusing than it's worth at this point. |
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.
Yeah, maybe it's not worth getting too detailed at this point. On the other hand, it helps me understand better what it would eventually look like. I'm kind of torn on whether it's worth it.
But you did most of it already, so why not finish it?
c878bea
to
daccbea
Compare
} | ||
|
||
dictionary VideoEncoderOutput { | ||
Uint8Array data; |
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.
I'm not sure it's good to switch from interfaces to dictionaries with (passive) data at this point. If we want to make efficient transformations that only look at the metadata (such as a layer-discarding filter), it may be better to leave the data as an accessor so that the case of passing frames unmodified can be handled without any data copying.
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.
This is already the case for the heavyweight resources (e.g., raw video frames) but not for bitstream contents. I think that discussion is outside the scope of this PR but would be valuable.
timestamp: ..., | ||
changeCodecSettings: { | ||
targetBitRate: 50_000, | ||
}, |
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.
Idea expressed in conversation: allow changing of codec settings even without a frame (WritableStream of either settings or video frame)
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.
explainer.md
Outdated
imageData: ..., | ||
timestamp: ..., | ||
frameSettings: { | ||
forceKeyFrame: true, |
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.
Not all frames can be key frames, so should this be a "make the next available frame a key frame" dynamic codec setting? Or should we expect the user of the API to always be smart enough to time it right?
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.
I was already having difficulty identifying frame settings, so I'm just going to remove the category. We can add it back later if it turns out to have use.
daccbea
to
c5bbe44
Compare
c5bbe44
to
486dc9c
Compare
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.
@pthatcherg I think this is good to land.
timestamp: ..., | ||
changeCodecSettings: { | ||
targetBitRate: 50_000, | ||
}, |
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.
} | ||
|
||
dictionary VideoEncoderOutput { | ||
Uint8Array data; |
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.
This is already the case for the heavyweight resources (e.g., raw video frames) but not for bitstream contents. I think that discussion is outside the scope of this PR but would be valuable.
explainer.md
Outdated
imageData: ..., | ||
timestamp: ..., | ||
frameSettings: { | ||
forceKeyFrame: true, |
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.
I was already having difficulty identifying frame settings, so I'm just going to remove the category. We can add it back later if it turns out to have use.
Addresses #5
PTAL @pthatcherg