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

Move SourceBufferWriteHead to Cobalt location #4851

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

osagie98
Copy link
Contributor

@osagie98 osagie98 commented Feb 5, 2025

  1. Moves the SourceBuffer.writeHead implementation from the mediasource module to the Cobalt module.
  2. Renames source_buffer_write_head.idl to source_buffer_extensions.idl.

b/395658866

@osagie98
Copy link
Contributor Author

osagie98 commented Feb 5, 2025

This is a quick look at how it may look when we move custom interface extensions to the Cobalt module. This can't build due to a dependency issue. However, I don't think we would need to deal with Mojo here. This also concerns #4837, which will need to do something similar once the process is defined

@hlwarriner
Copy link
Contributor

This looks like a good change.

Re Mojo, does SourceBufferWriteHead.writeHead() end up calling a Starboard API at some point in the call stack?

@osagie98
Copy link
Contributor Author

osagie98 commented Feb 6, 2025

It doesn't, writeHead goes as far as the ChunkDemuxerStream in the chromium media stack

@osagie98
Copy link
Contributor Author

Pending verification with the web platform test. This also needs a unit test - that can be addressed in a separate PR.

@osagie98 osagie98 marked this pull request as ready for review February 10, 2025 23:09
@osagie98 osagie98 requested a review from a team as a code owner February 10, 2025 23:09
Copy link
Contributor

@sideb0ard sideb0ard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good, I copied some of this structure for #4837 and it works well.

@osagie98
Copy link
Contributor Author

As WPTs aren't currently working, I'll verify them later. Also, ChunkDemuxerTests are broken/unverified and need extra work to get going. I think it should be fine to merge as is and verify through tests later. I'll still verify using a Cobalt demo before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants