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

Fix crash when using formats with bits per pixel size not divisible by 8 #61

Conversation

Wumpf
Copy link
Contributor

@Wumpf Wumpf commented Nov 5, 2024

Two separate fixes actually:

  • get_bytes_per_frame assumed that a bits per pixel size that is not divisible by 8 means that it can't retrieve the frame size because that will yield a frame buffer with a non full-byte size. This is not entirely true since for instance YUV formats typically have size restrictions such that their raw frame buffers don't run into this problem. ➡️ The check got changed to the frame buffer's size
  • don't panic when get_bytes_per_frame can't return a size and instead report the error and exit

The former fixes using yuv formats for me, the later is a bit of a general requirement for me since I deal with arbitrary user data: not being able to handle data is sad, but crashing over it goes too far :)

Copy link
Owner

@nathanbabcock nathanbabcock left a comment

Choose a reason for hiding this comment

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

Nice improvement to support YUV420P and potentially many other pixel formats as well. Also the better error handling is a welcome change.

There's an edge case here that needs to be handled: if the bytes per frame can't be calculated, it allocates a zero-size buffer and then the iterator constantly produces OutputFrame events with zero size and never finishes. That could happen, for example, with an irregularly-sized 321x421 YUV420P output.

I'll merge this as a starting point and follow up with a fallback to "chunked" mode — the same way it would handle encoded video data like H264. Depending on your use case that might be perfectly fine, or you may need to check for either OutputFrame or OutputChunk depending on the situation. We can iterate on this solution if needed.

@nathanbabcock nathanbabcock merged commit b8e50ab into nathanbabcock:main Nov 6, 2024
3 checks passed
@Wumpf
Copy link
Contributor Author

Wumpf commented Nov 6, 2024

if the bytes per frame can't be calculated, it allocates a zero-size buffer and then the iterator constantly produces OutputFrame events with zero size and never finishes

oops. I didn't notice that I only exit from the lambda that's passed to map and not the entire thread - I still wanted to handle this as a fatal error as before, just more gracefully.
Thank you so much for following up on this!

@Wumpf Wumpf deleted the fix-crash-on-non-8bit-divisible-formats branch November 6, 2024 09:49
Wumpf added a commit to rerun-io/rerun that referenced this pull request Nov 6, 2024
### What

* part of #7607

Recognizing ahead of time what format a piece of encoded data is
unfortunately not that easy. To accomplish that I fell into the rabbit
hole of parsing the Sequence Parameter Set (SPS). Which will hopefully
be useful for other things in the future!

Other than that, it turned out that determining the color space & color
range are again different cans of worms, so I'm being now a lot more
explicit with ffmpeg on what we expect.
Which.. is yet another rabbit hole! Pretty much no matter what I tried I
couldn't replicate exact colors some sample videos compared to VLC
(unlike we did with AV1 where we have a lot more solid grasp on all
these things and get satisfying parity!). But also here different video
players seem to do slightly different things, so I decided to not follow
this thread further for the moment as this whole affair already cost me
a lot more time that I wanted to invest originally.

~Partially disabled unfortunately since we need this fix:~
* nathanbabcock/ffmpeg-sidecar#61

<img width="390" alt="image"
src="https://github.com/user-attachments/assets/c340544a-0c32-406e-ba20-2acc630c0238">

(note that this uses 422, the video source material is 420 though)


Fixes slow 4k video handling:

Before:
<img width="512" alt="image"
src="https://github.com/user-attachments/assets/82a0f0a1-03e7-4eac-8a24-a17ba185a8b5">


After:
<img width="548" alt="image"
src="https://github.com/user-attachments/assets/fb8315f4-5a85-4c2e-83bb-7e7e0c8ded33">


### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/8002?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/8002?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!
* [x] If have noted any breaking changes to the log API in
`CHANGELOG.md` and the migration guide

- [PR Build Summary](https://build.rerun.io/pr/8002)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
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.

2 participants