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

Inaccurate docs/comments about ActiveStorage video previewer defaults could be improved #51802

Open
searls opened this issue May 13, 2024 · 4 comments
Labels

Comments

@searls
Copy link
Contributor

searls commented May 13, 2024

I spent way too long being confused why my app's video previews were not the first frame (despite documentation here, here, and here). This was how the video previewer behaved by default prior to 7.0, but was changed in #39096 to instead use ffmpeg scene detection to detect a more meaningful frame.

(This change actually results in worse video previews in my case, where the uploaded files are all vertical social media videos created with CapCut, whose cover image feature allows users to set the first frame of the video to a static image. As a result, scene detection will typically choose the subsequent frame as the cover image.)

Anyway, it'd be great if we could update the remaining references to "first frame" in all the code comments and guides for ActiveStorage, because it really threw me off the trail—especially since the Active Storage gem itself hard-codes the old default (-y -vframes 1 -f image2) in several spots, so I only discovered the new default by debugging the previewer.

I'm opening this as an issue instead of as a PR, because I don't know how to best succinctly summarize the new behavior in each of these spots. Should it link to the PR explaining the change?

@searls searls changed the title Inaccurate code comments about ActiveStorage video previewer defaults could be improved Inaccurate docs/comments about ActiveStorage video previewer defaults could be improved May 13, 2024
@zzak
Copy link
Member

zzak commented May 18, 2024

Is the same true if you aren't using ffmpeg with at least the supported version?

Meaning we probably should document both? 🤷

@searls
Copy link
Contributor Author

searls commented May 21, 2024

@zzak if I read correctly, I think the change effectively raised (or specified) the minimum required ffmpeg version to one that supported the new CLI args with 7.0 and was why it wasn't backported to 6.1

@zzak
Copy link
Member

zzak commented May 21, 2024

Ahh ok, #42471 helped me make sense of this. I missed that load_defaults hook.

The reason why the old default is hard-coded is to avoid a breaking change and to rely on load_defaults instead.

In hind-sight it might have been good to make accept? check the version, as I'm not sure how ubiquitous the supported version of ffmpeg was at the time.

Yeah we should update the doc in those places, it's safe to assume the docs are written with the knowledge of the current load_defaults already in place. The configuring guide is where the lore comes into play.

@zzak
Copy link
Member

zzak commented May 21, 2024

...uses scene change detection to generate preview images of videos.

Or something to that extent seems reasonable to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants