Refactor MaxImageDimensionSubscriber
#7850
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Actually, I just realized this means each video is parsed twice, which is… not great. I'll look into giving |
emilk
added
exclude from changelog
emilk
changed the title
MaxImageDimensionSubscriber
emilk
commented
Oct 21, 2024
Comment on lines
+155
to
+161
| if true { | ||
| None // TODO(#7821): Use the VideoCache here so we make sure we only load each video ONCE | ||
| } else { | ||
| re_video::VideoData::load_from_bytes(&blob, &media_type) | ||
| .ok() | ||
| .map(|video| video.dimensions()) | ||
| } |
There was a problem hiding this comment.
This is the core of it. I'll work on having access to VideoCache here, and then we can resolve this in a follow-up PR.
…or we add functionality to re_mp4 to very quickly parse an .mp4 to only look for its size info 🤔
That would be easy if we had:
(…and used memory-mapping)
This was referenced Oct 21, 2024
Wumpf
approved these changes
Oct 22, 2024
| max_dim.width = max_dim.width.max(width); | ||
| } | ||
| } | ||
| if let Some([width, height]) = size_from_blob(blob.as_ref(), media_type.as_deref()) |
There was a problem hiding this comment.
nit:
Suggested change
| if let Some([width, height]) = size_from_blob(blob.as_ref(), media_type.as_deref()) | |
| let Some([width, height]) = size_from_blob(blob.as_ref(), media_type.as_deref()) else { | |
| continue; | |
| }; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
The change is that
MaxImageDimensionSubscriberparses each.mp4to figure out its size……except I disabled that check last-second because we don't have access to the
VideoCache, and so this PR would lead to each video getting parsed twice.Still, I would like to get this PR merged as a necessary refactor and generally improved structure.
This is where we're heading:
Checklist
mainbuild: rerun.io/viewernightlybuild: rerun.io/viewerCHANGELOG.mdand the migration guideTo run all checks from
main, comment on the PR with@rerun-bot full-check.