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

Generate screenshot images for markers #1604

Merged
merged 5 commits into from
Sep 15, 2021

Conversation

gitgiggety
Copy link
Contributor

In some scenarios it might not be possible to use the preview video or
image of markers, i.e. when only static images are supported like in
Kodi. So generate a static screenshot as well.

@WithoutPants
Copy link
Collaborator

I don't really like the idea of generating artifacts for use exclusively outside of stash. This should probably be implemented as a separate plugin or script.

@gitgiggety
Copy link
Contributor Author

gitgiggety commented Aug 4, 2021

Hmm, unsure if / how this could be implemented as a plugin. Especially since it would need an API change (additional screenshot field) and the routing endpoint to actually download the image.

What I didn't investigate further, but might be worthwhile, is that the wall has the (static) image option which at the moment shows the video but doesn't auto play. Obviously this could use the static image as well. But unsure whether downloading a full size image would be faster / better than downloading a part of the preview video and showing the first frame as static image.
So maybe using the image generated by this branch as the static image option of the Wall view would convince you that there is a use case in Stash itself :)

Edit:
What I forgot. Or possibly implementing it as a separate task / option? So it isn't generated as part of the scene marker task (which currently generates both the preview video and webp preview image), but a new task Then users would have the choice whether they want this artifact to be generated.

@gitgiggety
Copy link
Contributor Author

@WithoutPants any update on this? Is there a possibility for you to reconsider this? :) As written before, it's impossible to do this in a plugin. And I'm open to making this a separate task so users can choose to generate these images instead of always generating them as part of the video preview generator task.

@WithoutPants
Copy link
Collaborator

We do indeed have a static image option.

There is a pending issue #588 related to adding the webp generation for markers. If you can add that option and a static image option, then we can have the marker wall view show the static image if it is selected in the interface options (I don't think it's a particularly useful option, but it is there).

@gitgiggety
Copy link
Contributor Author

So splitting the existing task in two (video at top level, webp as sub-option, just like scenes has), and adding the static image task as sub option as well.
And using the image as generated using this PR for the "static image" UI option as well.

👍

In some scenarios it might not be possible to use the preview video or
image of markers, i.e. when only static images are supported like in
Kodi. So generate a static screenshot as well.
@gitgiggety
Copy link
Contributor Author

Updated. Generating markers now has two subitems for animated images and static images. When wall is set to static image the static image will be used.

I made the decision to not generate the webp previews by default because they're also not generated for scene previews by default. But this obviously "breaks" backwards compatibility as previously they were always generated.

@WithoutPants WithoutPants added this to the Version 0.10.0 milestone Sep 14, 2021
@WithoutPants WithoutPants added the feature Pull requests that add a new feature label Sep 14, 2021
@WithoutPants WithoutPants merged commit 2274db1 into stashapp:develop Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull requests that add a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants