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

Refine Demuxer GC for low resolution videos #4483

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

jasonzhangxx
Copy link
Contributor

@jasonzhangxx jasonzhangxx commented Nov 22, 2024

The chunk demuxer garbage collection algorithm didn't account
bookkeeping data, which uses a significant amount of memory when
playing low resolution videos. For example, the reported memory usage of
ChunkDemuxerTest.WebMFile_AudioOnly test was 18624 bytes. After
including memory usage of 166 StreamParserBuffer instances, the new
memory usage increases to 69088 bytes.

This change is backported from m126+.

Fixed: b/372965149
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5353702
(cherry picked from commit 41153e57b9e6a7ef8dd2231dfc8cab201a956913)

@jasonzhangxx jasonzhangxx requested a review from a team as a code owner November 22, 2024 19:14
The chunk demuxer garbage collection algorithm didn't account
bookkeeping data, which uses a significant amount of memory when
playing low resolution videos. For example, the reported memory usage of
ChunkDemuxerTest.WebMFile_AudioOnly test was 18624 bytes. After
including memory usage of 166 StreamParserBuffer instances, the new
memory usage increases to 69088 bytes.

Fixed: b/372965149
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5353702
(cherry picked from commit 41153e57b9e6a7ef8dd2231dfc8cab201a956913)
@jasonzhangxx
Copy link
Contributor Author

Note that the code had some conflicts in media/base/decoder_buffer.cc, media/base/decoder_buffer.h, media/filters/source_buffer_state.cc and has been resolved.

@borongc
Copy link
Contributor

borongc commented Nov 22, 2024

Do we know which Chromium milestone contains this upstream PR, and maybe make a note here, so when we rebase to later milestones, we will know if this is needed?

@borongc
Copy link
Contributor

borongc commented Nov 22, 2024

It's available on m126+.

Maybe consider to include this cherry pick PR is available on m126+ in the commit message.

@jasonzhangxx
Copy link
Contributor Author

As it's cherry-picked from chromium repo, I'm not sure what git will do when rebasing to later chromium version which includes the original commit. The commit message is the same as in the original chromium change. @andrewsavage1, @dahlstrom-g, what do you think about Borong's comment?

@andrewsavage1
Copy link
Contributor

I'd defer to @dahlstrom-g about if we care about changing the original commit message, but it won't interfere with git history when we rebase if we do.

@kaidokert
Copy link
Member

As it's cherry-picked from chromium repo, I'm not sure what git will do when rebasing to later chromium version which includes the original commit.

Because we work off of squashed copy of upstream code, don't expect git to provide automatic resolution. We pretty much need to manually annotate and keep track of things we cherry-picked and can discard later.

Adding an extra line to the commit message doesn't seem like a bad idea, but we don't really have a convention captured anywhere for this yet.

@jasonzhangxx
Copy link
Contributor Author

jasonzhangxx commented Nov 25, 2024

Ok, added a line to explicitly mention it's from m126+.

@jasonzhangxx jasonzhangxx enabled auto-merge (squash) November 25, 2024 20:27
@andrewsavage1
Copy link
Contributor

Bypassing lint check, as it's incorrectly checking files unrelated to this change

@andrewsavage1 andrewsavage1 merged commit 8a934a6 into youtube:main Nov 25, 2024
57 of 59 checks passed
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.

4 participants