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 module #4851

Merged
merged 8 commits into from
Feb 13, 2025

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.

@hlwarriner
Copy link
Contributor

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.

Sounds good to me if you've tested locally and this works as expected. It would probably be good to have a bug to track the WPT; you could mark it as blocked by b/392940729.

@osagie98
Copy link
Contributor Author

Verified writeHead works with a demo

@osagie98 osagie98 changed the title Move SourceBufferWriteHead to Cobalt location Move SourceBufferWriteHead to Cobalt module Feb 12, 2025
Copy link
Contributor

@yell0wd0g yell0wd0g left a comment

Choose a reason for hiding this comment

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

Moving files LGTM!

@osagie98 osagie98 enabled auto-merge (squash) February 13, 2025 00:12
@osagie98 osagie98 merged commit 103051c into youtube:main Feb 13, 2025
143 checks passed
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.

5 participants