Preview PDFs and videos#30667
Preview PDFs and videos#30667georgeclaghorn merged 18 commits intorails:masterfrom georgeclaghorn:activestorage-previews
Conversation
dhh
left a comment
There was a problem hiding this comment.
This is really lovely. It's awesome that we can use the framework itself to provide additional features, like the preview.
| class ActiveStorage::PreviewsController < ActionController::Base | ||
| def show | ||
| if blob = find_signed_blob | ||
| expires_in 5.minutes # service_url defaults to 5 minutes |
There was a problem hiding this comment.
Should find a way to encode this information once. Maybe the service provider just sets this as a constant that we can reference here.
| end | ||
|
|
||
| def disposition_param | ||
| params[:disposition].presence_in(%w( inline attachment )) || "inline" |
There was a problem hiding this comment.
This is the third place we'd perform this piece of logic. Let's extract it into the Service with something like normalize_disposition or whatever. Then we can just rely on passing params[:disposition] straight in.
| end | ||
|
|
||
| private | ||
| def find_signed_blob |
There was a problem hiding this comment.
Don't think this method is worth it. Let's just inline in all three controllers.
| end | ||
|
|
||
| def decoded_variation | ||
| ActiveStorage::Variation.decode(params[:variation_key]) |
There was a problem hiding this comment.
Same story here. We can just have variant accept a variation_or_variation_key and then do the normalization there.
|
|
||
| class_attribute :service | ||
|
|
||
| has_one_attached :image |
There was a problem hiding this comment.
I think we need something a bit more explicit here, maybe even just preview_image would do?
| # extracting its first frame, and a PDF blob can be previewed by extracting its first page. | ||
| # | ||
| # Active Storage provides previewers for videos and PDFs. You can add your own previewers or remove the built-in ones | ||
| # by modifying +ActiveStorage.previewers+. |
There was a problem hiding this comment.
Maybe need a quick note that order of previewers matter, as it's first match that'll be used.
| end | ||
|
|
||
| def preview | ||
| open do |input| |
There was a problem hiding this comment.
So this will still require us to download the entire video file before we can start trying to do a preview. We should check what the biggest videos we have look like. I wouldn't be surprised if some of them are 20, 50, or even 100GB. It would kinda suck to have to download the whole file just to get the first frame for a preview.
I wonder if there's a way to explore whether some files can be used in truncated form. Like, could you just download the first 20mb and then do the preview off that?
There was a problem hiding this comment.
Per our discussion, we’re going to roll with this as-is. We’ll pursue performance optimizations if we find them necessary.
| ActiveStorage::Variant.new(image, variation).processed | ||
| end | ||
|
|
||
|
|
| # * {ffmpeg}(https://www.ffmpeg.org) | ||
| # * {mupdf}(https://mupdf.com) | ||
| # | ||
| # These libraries are not provided by Rails; to use the built-in previewers, you must install them yourself. |
There was a problem hiding this comment.
Are we detecting their presence early enough and raising helpful messages? It looks like this is currently setting people up for an unexpected error in production
There was a problem hiding this comment.
I think it's fine, because they'll fail as soon as you actually call #preview. Which will happen in test/dev. The upside of this is that it's a no config.
| file.rewind | ||
| end | ||
|
|
||
|
|
| mattr_accessor :verifier | ||
|
|
||
| mattr_accessor :previewers do | ||
| [ ActiveStorage::Previewer::PdfPreviewer, ActiveStorage::Previewer::VideoPreviewer ] |
There was a problem hiding this comment.
Why do we, and users who write their own previewer, have to add previewers here manually? Wouldn't an inherited hook work better?
|
Kasper, problem with that would be that you couldn't control the order.
Which you'll need to if you have some previewers matching specifics first,
then grand-catch later.
…On Fri, Sep 22, 2017 at 8:11 AM, Kasper Timm Hansen < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In activestorage/lib/active_storage.rb
<#30667 (comment)>:
>
mattr_accessor :verifier
+
+ mattr_accessor :previewers do
+ [ ActiveStorage::Previewer::PdfPreviewer, ActiveStorage::Previewer::VideoPreviewer ]
Why do we, and users who write their own previewer, have to add previewers
here manually? Wouldn't an inherited hook work better?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#30667 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAKtVMCJNg1xwX-4rxDUbyW4zj8KIY0ks5sk84FgaJpZM4PeQXn>
.
|
|
To install ffmpeg and mupdf on CI should be enough to add those lines to travis.yml: |
|
@dhh ah, so users are really meant to be able to do things like |
|
Yes 👍. Or rearrange the whole array, removing some they might not want in
there.
…On Sat, Sep 23, 2017 at 12:26 PM, Kasper Timm Hansen < ***@***.***> wrote:
@dhh <https://github.com/dhh> ah, so users are really meant to be able to
do things like ActiveStore.previewers.prepend SpecificPreviewer? And not
just append previewers?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#30667 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAKtSMkGEH0ZCN_u7br0kyl9Y4uSJrsks5slVt9gaJpZM4PeQXn>
.
|
|
I think this is a more widely-applicable observation for Active Storage, so may not be worth dealing with in this PR, but things like |
|
|
||
| class_attribute :service | ||
|
|
||
| class_attribute :default_url_expiry, default: 5.minutes |
There was a problem hiding this comment.
Don't think we often have configs that include default in the name. How about service_url_expiry? Or maybe service_url_expires_in since that connotes the relativeness of it and that users would want relative timestamps.
Alternatively, should this be a reader that wraps an ActiveStorage.service_url_expiry? (Which should then go through config.active_storage.service_url_expiry like @matthewd mentioned).
|
|
||
| class_attribute :logger | ||
|
|
||
| class_attribute :url_expires_in, default: 5.minutes |
There was a problem hiding this comment.
Neat 👍 — good call, fits better here.
|
I'm slightly surprised to see no warning about MuPDF's licensing. When I read this PR I hoped it was a more liberally licensed alternative to GhostScript for PDF processing, only to discover it's also Affero GPL. Unless the app developer buys a commercial license from Artifex, by using MuPDF they need to make their source code available per the license requirements. AFAIK most (if not all) Rails dependencies use MIT or similarly liberal licenses? |
|
Not an expert on this topic,
My understanding, please correct me if I am wrong
If you are using mupdf without altering the source and re distribution is not a concern, then you can you mupdf without a commercial license. But still a warning about license will be great |
|
I went thru AGPL meaning few years ago. TL;DR (sourced from wiki - https://en.wikipedia.org/wiki/Affero_General_Public_License)
If you use mupdf in Also https://choosealicense.com/licenses/agpl-3.0/ helps here:
IMHO @bdewater is right. It will be nice to find some alternative to mupdf to be optional dependency by default. |
|
Seems like we are using |
|
Artifex is clear that use of MuPDF in a closed-source Rails application requires a commercial license. See their licensing page:
I have a version of the PDF previewer that uses Apache PDFBox working locally. The main drawback is that it requires Java. If there are no objections, I’ll push this change. |
|
After some discussion, we’ve added a statement to the docs indicating that app developers should look into the licensing requirements that apply to them, since neither ffmpeg nor mupdf is released under the MIT License. This review of Artifex’s interpretation of the AGPL seems relevant. |
mutool uses AGPL for licensing which has strict distribution rules. rails#30667 (comment) Poppler is licensed under GPL, so this should provide a free alternative for most Rails apps.
Add a mechanism for generating and storing previews (images extracted from non-image blobs). Provide two built-in previewers: one that extracts the first frame from a video, and one that extracts the first page from a PDF document. The video previewer relies on FFmpeg and the PDF previewer relies on mupdf.
/cc @dhh