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 BufferAPI via Player.buffer #298

Merged
merged 54 commits into from
Oct 25, 2023
Merged

Conversation

123mpozzi
Copy link
Contributor

@123mpozzi 123mpozzi commented Oct 18, 2023

Description

Implement Buffer APIs to configure buffer settings and to query the current buffer state.

Changes

  • Add Player.buffer namespace
  • Implement BufferApi
  • Implement BufferType, BufferLevel, and MediaType
  • Add Buffer Module

Testing

Apply this patch, open Basic Playback, and look at the logs on Metro:
patch.diff.txt

Checks

  • buffer level updates correctly
  • on seeking, targetLevel is set to 300
  • media type and buffer type serialise correctly

Checklist

  • 🗒 CHANGELOG entry

@123mpozzi 123mpozzi self-assigned this Oct 18, 2023
@123mpozzi 123mpozzi changed the title Implement BufferAPI: namespace Implement BufferAPI: Add Player.buffer namespace Oct 18, 2023
@123mpozzi 123mpozzi marked this pull request as ready for review October 19, 2023 15:09
@123mpozzi 123mpozzi requested a review from zigavehovec October 19, 2023 15:12
@rolandkakonyi rolandkakonyi self-requested a review October 20, 2023 09:08
ios/BufferModule.swift Outdated Show resolved Hide resolved
src/bufferApi.ts Outdated Show resolved Hide resolved
src/bufferApi.ts Outdated Show resolved Hide resolved
src/bufferApi.ts Outdated Show resolved Hide resolved
src/bufferApi.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@rolandkakonyi rolandkakonyi left a comment

Choose a reason for hiding this comment

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

Good job!

Copy link
Contributor

@zigavehovec zigavehovec left a comment

Choose a reason for hiding this comment

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

Very nice improvements! 💪
I've added some additional comments. In my opinion this PR is ready after the 2 comments are addressed 🙂

as JSON handlers shouldn't handle logic, and this forces the kotlin
compiler to tell us to cover every case (so it is safer to handle
new cases added in the future)
@123mpozzi 123mpozzi requested a review from zigavehovec October 24, 2023 08:50
@123mpozzi 123mpozzi changed the title Implement BufferAPI: Add Player.buffer namespace Implement BufferAPI: Add Player.buffer Oct 25, 2023
@123mpozzi 123mpozzi changed the title Implement BufferAPI: Add Player.buffer Implement BufferAPI through Player.buffer Oct 25, 2023
We should take advantage of compilers to
get compile time errors when a new value isn't handled
instead of using default.
@123mpozzi 123mpozzi changed the title Implement BufferAPI through Player.buffer Implement BufferAPI via Player.buffer Oct 25, 2023
Copy link
Contributor

@zigavehovec zigavehovec left a comment

Choose a reason for hiding this comment

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

Looks good! Great job 💪

@123mpozzi 123mpozzi merged commit 8fce4a8 into development Oct 25, 2023
8 checks passed
@123mpozzi 123mpozzi deleted the bufferapi-namespace branch October 25, 2023 09:43
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.

3 participants