Skip to content

Conversation

@invario
Copy link
Contributor

@invario invario commented Mar 9, 2025

Hello, hopefully I did this correctly this time, thank you.

Summary

Previews currently generated for HDR videos result in desaturated/washed out images because of lack of tonemapping. This enhancement detects HDR videos and adds additional switches to the ffmpeg command to generate much improved images.

  • ffprobe is required (included with ffmpeg)
  • ffmpeg must have been built with libzimg (--enable-libzimg) for the zscale filter to work. This is platform/package dependent. (e.g. FreeBSD 13 does not. Alpine does. Unsure of others.)
  • adds configuration option preview_ffmpeg_enable_hdr
  • adds [optional] configuration option preview_ffprobe_path

Before
before

After
after

TODO

  • Perform actual check to see if ffmpeg was built with libzimg. However, this would require another proc_open with command ffmpeg -v quiet -filters and searching its output for 'zscale'. I don't think it's necessary as HDR preview generation would be disabled by default. Any user enabling the option should be aware of the requirement before doing so.
  • Update config.php.sample to show new options
  • Need additional testing

Checklist

@invario invario requested a review from a team as a code owner March 9, 2025 16:18
@invario invario requested review from icewind1991, skjnldsv and sorbaugh and removed request for a team March 9, 2025 16:18
@icewind1991
Copy link
Member

(disclaimer: my knowledge about ffmpeg/HDR is limited)

Is there a downside to using the hdr options for non-hdr content? It would be better to have hdr previews "just work" without having to do any special config would be better (assuming the ffmpeg has the proper support ofc).

Doing the extra exec to feature-detect seems fine to me, since we can also cache it if we're worried about the overhead. (Though the cost of ffmpeg -v quiet -filters is probably insignificant compared to the actual preview generation)

@invario
Copy link
Contributor Author

invario commented Mar 10, 2025

(disclaimer: my knowledge about ffmpeg/HDR is limited)

Is there a downside to using the hdr options for non-hdr content? It would be better to have hdr previews "just work" without having to do any special config would be better (assuming the ffmpeg has the proper support ofc).

Yes. The downside of using the HDR switches on an SDR video is that previews generated are degraded (extremely dark). That's why there is a need to differentiate. Do you need a sample image?

@icewind1991
Copy link
Member

I mean always doing the logic that is gated behind preview_ffmpeg_enable_hdr currently (so including the check for "smpte2084" or "arib-std-b67").

@invario
Copy link
Contributor Author

invario commented Mar 10, 2025

I mean always doing the logic that is gated behind preview_ffmpeg_enable_hdr currently (so including the check for "smpte2084" or "arib-std-b67").

Ah, sorry, misunderstood. No, there's no downside as long as we confirm somewhere earlier that ffmpeg was built with libzimg and ffprobe is also installed.

@icewind1991
Copy link
Member

Ok, I would be in favor then of always enabling the behavior and doing the feature detection

@invario
Copy link
Contributor Author

invario commented Mar 10, 2025

Ok, I would be in favor then of always enabling the behavior and doing the feature detection

I can write the routine to detect, and then have the value of preview_ffmpeg_enable_hdr set based on the results (instead of a user config option)?

Not sure where I should insert the routine for least performance impact. What did you mean by 'cache' the results earlier... I admit I don't know how to go about that.

@invario
Copy link
Contributor Author

invario commented Mar 10, 2025

Hang on actually, just figured out a more elegant way to do this. ffmpeg/ffprobe already display compile options which is output to stderr by default, unless suppressed by -v quiet.

I can use the same exec I used for ffprobe without the -v quiet and check its stderr for '--enable-libzimg'. No need to do a separate check.

@invario
Copy link
Contributor Author

invario commented Mar 10, 2025

There we go. Now checks for the presence of libzimg (zscale filter) when running ffprobe on the video.

@invario
Copy link
Contributor Author

invario commented Mar 11, 2025

@st3iny @joshtrichards Hello, I run jnto your names frequently when looking at preview generation related stuff. Would you mind taking a look? TIA!

@invario
Copy link
Contributor Author

invario commented Mar 19, 2025

I would love for this to gain some traction or if I can get some guidance on additional steps that I may be missing. Thank you.

@github-actions
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@icewind1991
Copy link
Member

@invario can you resolve the formatting issues from https://github.com/nextcloud/server/actions/runs/13774792167/job/38566161541?pr=51356

@invario
Copy link
Contributor Author

invario commented Mar 25, 2025

@invario can you resolve the formatting issues from https://github.com/nextcloud/server/actions/runs/13774792167/job/38566161541?pr=51356

Sorry, I'm a novice at this... working on it now, thank you.

@invario
Copy link
Contributor Author

invario commented Mar 25, 2025

Okay, I think I fixed the formatting. Apologies for the improper commit message, did not realize it would push here. (I'm learning!)

nextcloud-bot and others added 6 commits March 25, 2025 19:30
Previews currently generated for HDR videos result in desaturated/washed out images because of lack of tonemapping. This enhancement detects HDR videos and adds additional switches to the ffmpeg command to generate much improved images.

Signed-off-by: invario <[email protected]>
Signed-off-by: KT <[email protected]>
Signed-off-by: invario <[email protected]>
Signed-off-by: KT <[email protected]>
- removed preview_ffmpeg_enable_hdr option and instead check ffmpeg build options for libzimg and enable/disable based on that

Signed-off-by: invario <[email protected]>
Signed-off-by: KT <[email protected]>
Use of str_pos on null would result if ffprobe failed to exec.

Signed-off-by: invario <[email protected]>
Signed-off-by: KT <[email protected]>
Signed-off-by: KT <[email protected]>
@invario invario requested review from susnux and removed request for a team March 25, 2025 23:31
@invario
Copy link
Contributor Author

invario commented Mar 25, 2025

Once again, apologies, but I'm making a mess of things. :(

@invario
Copy link
Contributor Author

invario commented Mar 25, 2025

I'm going to do a new PR and close this one out.

@invario invario closed this Mar 25, 2025
@invario invario changed the title Enhance: Better previews for HDR video Enhance: Better previews for HDR video (CLOSED, IGNORE) May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Previews of HDR videos being generated are washed out (needs tonemap?)

3 participants