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

Use dynamic allocation for ffmpeg fft tables on Windows. #21539

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

goodov
Copy link
Member

@goodov goodov commented Jan 10, 2024

This PR:

  1. replaces static arrays with on-demand dynamic memory allocations.
  2. reverts unnecessary 3.5MB allocations that has no observable effects on Chromium codebase. FFmpeg/FFmpeg@8f48a62. The patch [1/5] was discussed and landed here: https://patchwork.ffmpeg.org/project/ffmpeg/patch/20230616222048.6562-2-michael@niedermayer.cc/

Other related patches around avutil/tx_template look fine and shouldn't break because of our revert:

[2/5] https://patchwork.ffmpeg.org/project/ffmpeg/patch/20221206234808.10024-2-michael@niedermayer.cc/
[3/5] https://patchwork.ffmpeg.org/project/ffmpeg/patch/AS8P250MB074401A2AF976238F94F118B8F2C9@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM/
[4/5] https://patchwork.ffmpeg.org/project/ffmpeg/patch/AS8P250MB0744B3A7C36BC043E7E56B388F2C9@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM/
[5/5] https://patchwork.ffmpeg.org/project/ffmpeg/patch/AS8P250MB0744DF70DF80AEA5896DF0598F2C9@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM/

The original issue is only reproducible on Windows, because Windows maps bss_seg to private memory even if the memory is not accessed. Linux on the other hand does the mapping only on memory access.

Resolves brave/brave-browser#35264

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@github-actions github-actions bot added the CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) label Jan 10, 2024
@goodov goodov marked this pull request as ready for review January 11, 2024 09:24
@goodov goodov requested review from a team and bridiver as code owners January 11, 2024 09:24
+#define ALLOCATE_FF_TX_TABLE(len)
+
+#endif
+
Copy link
Member Author

Choose a reason for hiding this comment

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

we cannot use chromium_src for this override, because tx_template.c is included into tx_float.c.

- SR_TABLE(262144) \
- SR_TABLE(524288) \
- SR_TABLE(1048576) \
- SR_TABLE(2097152) \
Copy link
Collaborator

@atuchin-m atuchin-m Jan 11, 2024

Choose a reason for hiding this comment

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

Let's add the link to the FFmpeg discussion about adding these lines. this PR.
It's hard to find it from the paches blame.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's added into description
image

@goodov goodov merged commit 7a679b1 into master Jan 15, 2024
21 checks passed
@goodov goodov deleted the ffmpeg-dynamic-alloc branch January 15, 2024 10:22
@github-actions github-actions bot added this to the 1.63.x - Nightly milestone Jan 15, 2024
mkarolin pushed a commit that referenced this pull request Jan 17, 2024
Use dynamic allocation for ffmpeg fft tables on Windows.
kjozwiak pushed a commit that referenced this pull request Jan 20, 2024
* Merge pull request #21134 from brave/cr121

Upgrade from Chromium 120 to Chromium 121.

* Remove the assert for patch_ffmpeg.py (#21184)

* Merge pull request #21539 from brave/ffmpeg-dynamic-alloc

Use dynamic allocation for ffmpeg fft tables on Windows.

* Merge pull request #21585 from brave/issues/35318

Remove dynamic allocation of ffmpeg ff_tx tables.

* Disables a flaky upstream browser test.

* Merge pull request #21584 from brave/fix_new_tab_button_plus_misaligned

Fixed new tab button's plus icon is mis-aligned with horizontal tab

* Merge pull request #21600 from brave/121.0.6167.75_master

Upgrade from Chromium 121.0.6167.57 to Chromium 121.0.6167.75.

* Merge pull request #21628 from brave/maxk-disable-reading-mode

Hides `Open in Reading Mode` context menu item.

* [Uplift 1.62.x] AI chat issues cr121 1.62.x (#21629)

* aichat: input is growable (#21124)

* aichat: scroll is interruptable (#21235)

* aichat: model maker text shouldnt look like a link (#21220)

* aichat: code formatting (#21342)

* make claude output formatted code (#21599)

---------

Co-authored-by: Mikhail <matuchin@brave.com>
Co-authored-by: Aleksey Khoroshilov <5928869+goodov@users.noreply.github.com>
Co-authored-by: Simon Hong <shong@brave.com>
Co-authored-by: taher <8665427+nullhook@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve ffmpeg-triggered Windows memory usage issues
3 participants