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

Add maxVideoBufferSize option to limit the amount of video data (in kilobytes?) the RxPlayer will push on low-end devices #1041

Merged
merged 13 commits into from
Mar 17, 2022

Conversation

achrafl0
Copy link
Collaborator

@achrafl0 achrafl0 commented Jan 5, 2022

No description provided.

@peaBerberian
Copy link
Collaborator

peaBerberian commented Jan 6, 2022

Because one RepresentationStream is created per-type of buffer (one audio, one video, one text...), I'm under the impression that the current code could possibly lead to number_of_buffers * buffer_size, no?

In any case, I'm wondering if that buffer size setting would not be better per-type: especially one for audio and one for video.
What do you think?

src/manifest/index.ts Outdated Show resolved Hide resolved
@peaBerberian
Copy link
Collaborator

Can you also rebase on master so the new GitHub actions are run?

@achrafl0 achrafl0 changed the base branch from misc/inventory-segmentSize to master January 13, 2022 14:12
@peaBerberian peaBerberian added this to the 3.27.0 milestone Jan 13, 2022
@peaBerberian peaBerberian added API Relative to the RxPlayer's API Compatibility Relative to the RxPlayer's compatibility with various devices and environments enhancement This is a new feature and/or behavior which brings an improvement to the RxPlayer and removed enhancement This is a new feature and/or behavior which brings an improvement to the RxPlayer labels Jan 13, 2022
@peaBerberian
Copy link
Collaborator

peaBerberian commented Feb 10, 2022

Almost done.
To me, there's still:

  1. the config documentation to update.
  2. The point about gcedPosition to clarify
  3. the api documentation to add (both in /doc/api/Creating_a_Player.md for constructor options and I guess in the /doc/api/Buffer_Control directory for the buffer size methods)
  4. I would like that we first merge performance tests (Add performance tests #1062) so we can officially use them here before merging

@peaBerberian peaBerberian changed the base branch from master to next February 10, 2022 10:30
@peaBerberian
Copy link
Collaborator

I also updated the base branch to next as we usually do for new features (the idea is that, in a case of a huge issue in production, a less risky patch version without any new feature can be derived from the master branch)

@peaBerberian peaBerberian added the skip-performance-checks Performance tests are not run on this PR label Feb 14, 2022
src/config.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
@peaBerberian peaBerberian removed the skip-performance-checks Performance tests are not run on this PR label Feb 14, 2022
@peaBerberian
Copy link
Collaborator

When adding documentation files, you have also to update the .docConfig.json file in that directory so that the documentation generator knows the order the page have to be shown in.

Without it, the corresponding page won't be converted into HTML.

@peaBerberian peaBerberian added the skip-performance-checks Performance tests are not run on this PR label Feb 15, 2022
@achrafl0 achrafl0 force-pushed the feat/add-memory-size-buffer branch from fc25c86 to 775d48c Compare March 2, 2022 09:21
@peaBerberian peaBerberian added skip-performance-checks Performance tests are not run on this PR and removed skip-performance-checks Performance tests are not run on this PR labels Mar 9, 2022
@@ -19,7 +19,7 @@ this job instead.

<div class="warning">
This option will have no effects if we didn't buffer at least <b>MIN_BUFFER_LENGTH</b>
<i>( defaults at 5sec )</i>
<i>( defaults at 5sec )</i>.
Copy link
Collaborator

@peaBerberian peaBerberian Mar 16, 2022

Choose a reason for hiding this comment

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

I think we shouldn't expose the name of our internal variable for two reasons:

  • it might confuse the user
  • it might change at any new version, without us thinking to update that part of the documentation

@peaBerberian
Copy link
Collaborator

Needs to be rebased on next (I supposed since the config PR has been merged)

@achrafl0 achrafl0 merged commit b30389d into next Mar 17, 2022
@peaBerberian peaBerberian mentioned this pull request Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Relative to the RxPlayer's API Compatibility Relative to the RxPlayer's compatibility with various devices and environments Priority: 1 (High) This issue or PR has a high priority. skip-performance-checks Performance tests are not run on this PR
Projects
None yet
2 participants