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

Implement SourceBuffer.writeHead attribute #4559

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

Conversation

osagie98
Copy link
Contributor

@osagie98 osagie98 commented Dec 11, 2024

This change implements a partial interface for SourceBuffer, adding the writeHead attribute. This attribute returns the highest buffer presentation timestamp written to the SbPlayer. The writeHead attribute is only included in Cobalt Starboard media builds.

Link to the original PR with most of the functional changes: #121

b/326652276

This change implements a partial interface for SourceBuffer,
adding the writeHead attribute. This attribute returns the highest
buffer presentation timestamp written to the SbPlayer.
The writeHead attribute is guarded by a Runtime Enabled Feature
SourceBufferWriteHead. This feature is enabled by default for Cobalt
builds and disabled for all other builds.

Link to the original PR with most of the functional changes:
youtube#121

b/326652276
@osagie98
Copy link
Contributor Author

Just realized that since the new interface is only included in Cobalt Starboard media builds, we may not need to guard it by a RuntimeEnabledFeature. I can remove that if needed

@osagie98 osagie98 requested a review from borongc December 11, 2024 22:56
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.

Looks pretty straight forward

Copy link
Contributor

@andrewsavage1 andrewsavage1 left a comment

Choose a reason for hiding this comment

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

@@ -416,6 +423,12 @@ class MEDIA_EXPORT ChunkDemuxer : public Demuxer {
base::TimeDelta currentMediaTime,
size_t newDataSize);

#if BUILDFLAG(USE_STARBOARD_MEDIA)
// Returns the latest presentation timestamp of the buffers sent to the
// DemuxerStream.
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider rephrasing, as the buffers are read from the DemuxerStream instead of sending to the DemuxerStream. Please also rephrase the comment above for GetWriteHead().

@@ -140,6 +144,11 @@ class WebSourceBuffer {
// After this method is called, this WebSourceBuffer should never use the
// client pointer passed to SetClient().
virtual void RemovedFromMediaSource() = 0;

#if BUILDFLAG(USE_STARBOARD_MEDIA)
// Return the highest presentation timestamp written to the SbPlayer.
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have to mention SbPlayer here, as the implementation is no longer SbPlayer specific.

The same to the commit message of the PR.

#if BUILDFLAG(USE_STARBOARD_MEDIA)
// Cobalt-specific method that returns the highest presentation
// timestamp written to the SbPlayer.
double writeHead(ExceptionState& exception_state) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

getWriteHead()? Since it's no longer a method mapped from DOM. Please feel free to keep it as is if this is a convention in Chromium.

// from the MediaSource object.

[
ImplementedAs=SourceBufferWriteHead
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure the best convention for file names and class name here, i.e. shall we use SourceBufferWriteHead or CobaltSourceBuffer (and source_buffer_write_head.idl or cobalt_source_buffer.idl).

Just to bring this up, and please feel free to keep this as is for now or bring this to an internal sync for discussion.

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

Successfully merging this pull request may close these issues.

5 participants