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 document config.active_storage.video_preview_arguments description. [skip ci] #44060

Conversation

soartec-lab
Copy link
Contributor

Summary

Removed default value and move arguments description.
config.active_storage.video_preview_arguments

Following the discussion of #39387, omit the default value config.active_storage.video_preview_arguments from the description of the settings. This is because it depends on config.load_defaults.

However, I think the rails 7.0 settings description is a good way to help developers understand, so I moved this description to the version 7.0 default settings section.

@rails-bot rails-bot bot added the docs label Jan 4, 2022
@jonathanhefner
Copy link
Member

I agree that the explanation is helpful, and we should keep it. But it feels out of place in the long list of default values.

Since the discussion in #44029, I have proposed #44038 to prevent future confusion. With that PR, we could render:

video_preview_arguments-p

(Source Code)
| Starting with version | The default value is |
| --------------------- | -------------------- |
| baseline              | `"-y -vframes 1 -f image2"` |
| 7.0                   | `"-vf 'select=eq(n\\,0)+eq(key\\,1)+gt(scene\\,0.015)"` <p><em>Select the first video frame, plus keyframes, plus frames that meet the scene change threshold.</em></p> `+ ",loop=loop=-1:size=2,trim=start_frame=1'"` <p><em>Use the first video frame as a fallback when no other frames meet the criteria by looping the first (one or) two selected frames, then dropping the first looped frame.</em></p> `+ " -frames:v 1 -f image2"` |

Or, perhaps even:

video_preview_arguments-sup

(Source Code)
| Starting with version | The default value is |
| --------------------- | -------------------- |
| baseline              | `"-y -vframes 1 -f image2"` |
| 7.0                   | `"-vf 'select=eq(n\\,0)+eq(key\\,1)+gt(scene\\,0.015)"`<sup><strong><em>1</em></strong></sup> <br> `+ ",loop=loop=-1:size=2,trim=start_frame=1'"`<sup><strong><em>2</em></strong></sup><br> `+ " -frames:v 1 -f image2"` <br><br> <ol><li>Select the first video frame, plus keyframes, plus frames that meet the scene change threshold.</li> <li>Use the first video frame as a fallback when no other frames meet the criteria by looping the first (one or) two selected frames, then dropping the first looped frame.</li></ol> |

In the mean time, I think we could just make an exception:

diff --git a/guides/source/configuring.md b/guides/source/configuring.md
index 5efe31b6bb..4554ace23f 100644
--- a/guides/source/configuring.md
+++ b/guides/source/configuring.md
@@ -1733,14 +1733,16 @@ The default is `:rails_storage_redirect`.

 Can be used to alter the way ffmpeg generates video preview images.

-By default, this is defined as:
+With `config.load_defaults 7.0`, the default is:

 ```ruby
 config.active_storage.video_preview_arguments = "-vf 'select=eq(n\\,0)+eq(key\\,1)+gt(scene\\,0.015),loop=loop=-1:size=2,trim=start_frame=1' -frames:v 1 -f image2"
 ```

+Which has the following behavior:
+
 1. `select=eq(n\,0)+eq(key\,1)+gt(scene\,0.015)`: Select the first video frame, plus keyframes, plus frames that meet the scene change threshold.
-2. `loop=loop=-1:size=2,trim=start_frame=1`: To use the first video frame as a fallback when no other frames meet the criteria, loop the first (one or) two selected frames, then drop the first looped frame.
+2. `loop=loop=-1:size=2,trim=start_frame=1`: Use the first video frame as a fallback when no other frames meet the criteria by looping the first (one or) two selected frames, then dropping the first looped frame.

 #### `config.active_storage.multiple_file_field_include_hidden`

@soartec-lab soartec-lab force-pushed the fix/doc-video_preview_arguments-default-value branch from 68737a4 to e237170 Compare January 4, 2022 23:37
@soartec-lab
Copy link
Contributor Author

@jonathanhefner
Thank you for tell me. Your suggestion #44038 also looks good.
Regarding this PR, I have shown in the note that it is the default value of config.load_defaults 7.0 like the source code you provided.

could you review again ?

@jonathanhefner jonathanhefner force-pushed the fix/doc-video_preview_arguments-default-value branch from e237170 to daff381 Compare January 5, 2022 18:26
@jonathanhefner jonathanhefner merged commit 2e5ed2f into rails:main Jan 5, 2022
@jonathanhefner
Copy link
Member

jonathanhefner commented Jan 5, 2022

Looks good. I also added the last few lines of the diff (the part with "Which has the following behavior"). Thank you, @soartec-lab! 👍

Backported to 7-0-stable in 57edcf2.

jonathanhefner added a commit that referenced this pull request Jan 5, 2022
…ments-default-value

Fix document `config.active_storage.video_preview_arguments` description. [ci-skip]

(cherry picked from commit 2e5ed2f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants