-
Notifications
You must be signed in to change notification settings - Fork 175
media: Expose kMaxPendingBytesPerParse to H5vcc #8511
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
base: 26.android
Are you sure you want to change the base?
media: Expose kMaxPendingBytesPerParse to H5vcc #8511
Conversation
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
e095a5a to
3244844
Compare
Bug: 471099187
3244844 to
46a7a99
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request exposes media::StreamParser::kMaxPendingBytesPerParse as an H5vcc setting. The implementation is mostly correct, but there's a significant issue where the value from the setting, which is in kilobytes, is not converted to bytes before being used. This would cause the setting to be misinterpreted by a factor of 1024. I've also pointed out a minor style issue regarding vertical whitespace. Please see my detailed comments.
| int experimental_increment_size = *val; | ||
| media::StreamParser::SetMaxPendingBytesPerParse( | ||
| experimental_increment_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The H5vcc setting Media.SourceBufferIncrementalAppendSizeKb provides a value in kilobytes, as indicated by the Kb suffix in its name. However, the code passes this value directly to SetMaxPendingBytesPerParse, which expects a value in bytes. This will cause the setting to be misinterpreted, using a value 1024 times smaller than intended.
Please convert the value from kilobytes to bytes by multiplying it by 1024. I've also updated the variable name for clarity.
int experimental_increment_size_kb = *val;
media::StreamParser::SetMaxPendingBytesPerParse(
experimental_increment_size_kb * 1024);| #if BUILDFLAG(USE_STARBOARD_MEDIA) | ||
| // We drop the use of constexpr here to allow for experimentation. | ||
| static int kMaxPendingBytesPerParse; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the Chromium C++ style guide's principle of minimizing vertical whitespace, please remove this blank line. This will also make the code consistent with the surrounding member declarations in this class.
References
- The repository style guide requires adherence to the Chromium style guide. The Chromium style guide advises minimizing vertical whitespace and avoiding unnecessary blank lines between member declarations. (link)
Bug: 471099187