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

Simplify fast scenechange code a bit #3320

Merged
merged 3 commits into from
Dec 29, 2023

Conversation

FreezyLemon
Copy link
Contributor

downscaled_frame_buffer is an Option<([Plane<T>; 2], bool)>. The bool is called is_initialized and is supposed to show whether the memory in the [Plane<T>; 2] is initialized or not.

As far as I can tell, there is no code path that results in the Planes being uninitialized: downscaled_frame_buffer starts out as a None, the first buffer access allocates & initializes the memory, and the memory should always be initialized after that. The bool never seems to be false from what I can tell, which would support my assumption, I think.

There's also a field called frame_ref_buffer which caches the Arc<Frame<T>> when no downscaling is being done. Unless I am missing something, there's no real benefit to caching these pointers (because refs to both frames will always be passed into the function anyways), so i removed the buffer.

I tested my changes to the best of my ability, and didn't notice any change in behaviour or performance. LMK what you think

Copy link

codecov bot commented Dec 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3374dc9) 88.48% compared to head (5f127b6) 88.48%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3320      +/-   ##
==========================================
- Coverage   88.48%   88.48%   -0.01%     
==========================================
  Files          88       88              
  Lines       28236    28235       -1     
==========================================
- Hits        24986    24985       -1     
  Misses       3250     3250              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@barrbrain
Copy link
Collaborator

@shssoichiro What are the implications for https://github.com/rust-av/av-scenechange/ ?

@shssoichiro
Copy link
Collaborator

shssoichiro commented Dec 29, 2023

It looks like this shouldn't break the scene detection code in either av-scenechange or av1an, as far as I can tell.

@shssoichiro
Copy link
Collaborator

shssoichiro commented Dec 29, 2023

As for why the code is like this, from what I recall at one point in time I believed there to be cases where we may be re-running a comparison on the same pair of frames twice. However, it's possible that is no longer true (or never was true) if there is no measurable difference in performance. This shouldn't affect behavior either way, as it was intended to be a performance optimization, but I'm okay with removing it.

Both frames are always supplied as parameters,
so it probably doesn't make sense to cache these
@barrbrain barrbrain force-pushed the simplify-fast-scenechange branch from 5dfcbcd to 5f127b6 Compare December 29, 2023 23:33
@barrbrain barrbrain merged commit f8d82b7 into xiph:master Dec 29, 2023
@FreezyLemon FreezyLemon deleted the simplify-fast-scenechange branch December 30, 2023 00:30
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