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

Preview PDFs and videos #30667

Merged
merged 18 commits into from Sep 28, 2017

Conversation

Projects
None yet
9 participants
@georgeclaghorn
Member

georgeclaghorn commented Sep 20, 2017

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

@georgeclaghorn georgeclaghorn changed the title from Preview PDFs and videos to Preview PDFs and videos (WIP) Sep 20, 2017

@dhh

This is really lovely. It's awesome that we can use the framework itself to provide additional features, like the preview.

Show outdated Hide outdated activestorage/app/controllers/active_storage/previews_controller.rb Outdated
Show outdated Hide outdated activestorage/app/controllers/active_storage/previews_controller.rb Outdated
Show outdated Hide outdated activestorage/app/controllers/active_storage/previews_controller.rb Outdated
Show outdated Hide outdated activestorage/app/controllers/active_storage/previews_controller.rb Outdated
Show outdated Hide outdated activestorage/app/models/active_storage/blob.rb Outdated
Show outdated Hide outdated activestorage/app/models/active_storage/preview.rb Outdated
end
def preview
open do |input|

This comment has been minimized.

@dhh

dhh Sep 21, 2017

Member

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?

@dhh

dhh Sep 21, 2017

Member

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?

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Sep 28, 2017

Member

Per our discussion, we’re going to roll with this as-is. We’ll pursue performance optimizations if we find them necessary.

@georgeclaghorn

georgeclaghorn Sep 28, 2017

Member

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

This comment has been minimized.

@sgrif

sgrif Sep 21, 2017

Member

✂️

@sgrif

sgrif Sep 21, 2017

Member

✂️

This comment has been minimized.

@dhh

dhh Sep 21, 2017

Member

Intentional CR for separation.

@dhh

dhh Sep 21, 2017

Member

Intentional CR for separation.

Show outdated Hide outdated activestorage/app/models/active_storage/preview.rb Outdated
file.rewind
end

This comment has been minimized.

@sgrif

sgrif Sep 21, 2017

Member

✂️

@sgrif

sgrif Sep 21, 2017

Member

✂️

This comment has been minimized.

@dhh

dhh Sep 21, 2017

Member

Intentional CR for separation.

@dhh

dhh Sep 21, 2017

Member

Intentional CR for separation.

@dhh

Beautiful 👌

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Sep 22, 2017

Member
Member

dhh commented Sep 22, 2017

@simi

This comment has been minimized.

Show comment
Hide comment
@simi

simi Sep 22, 2017

Contributor

To install ffmpeg and mupdf on CI should be enough to add those lines to travis.yml:

addons: 
  apt: 
    sources: 
      - sourceline: 'ppa:mc3man/trusty-media' 
    packages: 
      - ffmpeg 
      - mupdf 
Contributor

simi commented Sep 22, 2017

To install ffmpeg and mupdf on CI should be enough to add those lines to travis.yml:

addons: 
  apt: 
    sources: 
      - sourceline: 'ppa:mc3man/trusty-media' 
    packages: 
      - ffmpeg 
      - mupdf 
@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 23, 2017

Member

@dhh ah, so users are really meant to be able to do things like ActiveStore.previewers.prepend SpecificPreviewer? And not just append previewers?

Member

kaspth commented Sep 23, 2017

@dhh ah, so users are really meant to be able to do things like ActiveStore.previewers.prepend SpecificPreviewer? And not just append previewers?

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Sep 23, 2017

Member
Member

dhh commented Sep 23, 2017

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Sep 24, 2017

Member

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 previewers need to go through config

Member

matthewd commented Sep 24, 2017

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 previewers need to go through config

georgeclaghorn added some commits Sep 25, 2017

@@ -42,6 +43,8 @@ class Service
class_attribute :logger
class_attribute :url_expires_in, default: 5.minutes

This comment has been minimized.

@kaspth

kaspth Sep 25, 2017

Member

Neat 👍 — good call, fits better here.

@kaspth

kaspth Sep 25, 2017

Member

Neat 👍 — good call, fits better here.

@bdewater

This comment has been minimized.

Show comment
Hide comment
@bdewater

bdewater Sep 26, 2017

Contributor

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?

Contributor

bdewater commented Sep 26, 2017

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?

@redtachyons

This comment has been minimized.

Show comment
Hide comment
@redtachyons

redtachyons Sep 26, 2017

Not an expert on this topic,
Section 13 of AGPL

  1. Remote Network Interaction; Use with the GNU General Public License.

Notwithstanding any other provision of this License, if you modify the Program, your modified version must prominently offer all users interacting with it remotely through a computer network (if your version supports such interaction) an opportunity to receive the Corresponding Source of your version by providing access to the Corresponding Source from a network server at no charge, through some standard or customary means of facilitating copying of software. This Corresponding Source shall include the Corresponding Source for any work covered by version 3 of the GNU General Public License that is incorporated pursuant to the following paragraph.

Notwithstanding any other provision of this License, you have permission to link or combine any covered work with a work licensed under version 3 of the GNU General Public License into a single combined work, and to convey the resulting work. The terms of this License will continue to apply to the part which is the covered work, but the work with which it is combined will remain governed by version 3 of the GNU General Public License.

My understanding, please correct me if I am wrong
Without a commercial license of mupdf

  1. You can't distribute your rails application with mupdf bundled with less restrictive or closed source license

  2. You can't change the source of code of mupdf and use it in your own rails app server(a typical saas app setup)

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

redtachyons commented Sep 26, 2017

Not an expert on this topic,
Section 13 of AGPL

  1. Remote Network Interaction; Use with the GNU General Public License.

Notwithstanding any other provision of this License, if you modify the Program, your modified version must prominently offer all users interacting with it remotely through a computer network (if your version supports such interaction) an opportunity to receive the Corresponding Source of your version by providing access to the Corresponding Source from a network server at no charge, through some standard or customary means of facilitating copying of software. This Corresponding Source shall include the Corresponding Source for any work covered by version 3 of the GNU General Public License that is incorporated pursuant to the following paragraph.

Notwithstanding any other provision of this License, you have permission to link or combine any covered work with a work licensed under version 3 of the GNU General Public License into a single combined work, and to convey the resulting work. The terms of this License will continue to apply to the part which is the covered work, but the work with which it is combined will remain governed by version 3 of the GNU General Public License.

My understanding, please correct me if I am wrong
Without a commercial license of mupdf

  1. You can't distribute your rails application with mupdf bundled with less restrictive or closed source license

  2. You can't change the source of code of mupdf and use it in your own rails app server(a typical saas app setup)

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

@simi

This comment has been minimized.

Show comment
Hide comment
@simi

simi Sep 26, 2017

Contributor

I went thru AGPL meaning few years ago.

TL;DR (sourced from wiki - https://en.wikipedia.org/wiki/Affero_General_Public_License)

This provision requires that the full source code be made available to any network user of the AGPL-licensed work, typically a web application.

If you use mupdf in Rails app, any application user gains right to obtain Rails app source code.

Also https://choosealicense.com/licenses/agpl-3.0/ helps here:

Network use is distribution (Users who interact with the software via network are given the right to receive a copy of the source code.)

Disclose source (Source code must be made available when the software is distributed.)

IMHO @bdewater is right. It will be nice to find some alternative to mupdf to be optional dependency by default.

Contributor

simi commented Sep 26, 2017

I went thru AGPL meaning few years ago.

TL;DR (sourced from wiki - https://en.wikipedia.org/wiki/Affero_General_Public_License)

This provision requires that the full source code be made available to any network user of the AGPL-licensed work, typically a web application.

If you use mupdf in Rails app, any application user gains right to obtain Rails app source code.

Also https://choosealicense.com/licenses/agpl-3.0/ helps here:

Network use is distribution (Users who interact with the software via network are given the right to receive a copy of the source code.)

Disclose source (Source code must be made available when the software is distributed.)

IMHO @bdewater is right. It will be nice to find some alternative to mupdf to be optional dependency by default.

@redtachyons

This comment has been minimized.

Show comment
Hide comment
@redtachyons

redtachyons Sep 26, 2017

Seems like we are using mutool from shell here and it will be MereAggregation as per GPL terms, So I believe this usage won't make an issue in terms of licensing

redtachyons commented Sep 26, 2017

Seems like we are using mutool from shell here and it will be MereAggregation as per GPL terms, So I believe this usage won't make an issue in terms of licensing

@georgeclaghorn

This comment has been minimized.

Show comment
Hide comment
@georgeclaghorn

georgeclaghorn Sep 26, 2017

Member

Artifex is clear that use of MuPDF in a closed-source Rails application requires a commercial license. See their licensing page:

Generally speaking, “distribution” is the key word. Some examples of distribution/network use requiring a commercial license include:

[...]

Running Ghostscript or MuPDF on your network servers and permitting non-employees access to interact with these products as part of your non-AGPL application. Example: utilizing Ghostscript to interpret and display a PDF on a cloud application.

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.

Member

georgeclaghorn commented Sep 26, 2017

Artifex is clear that use of MuPDF in a closed-source Rails application requires a commercial license. See their licensing page:

Generally speaking, “distribution” is the key word. Some examples of distribution/network use requiring a commercial license include:

[...]

Running Ghostscript or MuPDF on your network servers and permitting non-employees access to interact with these products as part of your non-AGPL application. Example: utilizing Ghostscript to interpret and display a PDF on a cloud application.

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.

@georgeclaghorn

This comment has been minimized.

Show comment
Hide comment
@georgeclaghorn

georgeclaghorn Sep 26, 2017

Member

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.

Member

georgeclaghorn commented Sep 26, 2017

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.

@georgeclaghorn georgeclaghorn changed the title from Preview PDFs and videos (WIP) to Preview PDFs and videos Sep 28, 2017

@georgeclaghorn georgeclaghorn merged commit d305862 into rails:master Sep 28, 2017

1 check passed

codeclimate All good!
Details

@georgeclaghorn georgeclaghorn deleted the georgeclaghorn:activestorage-previews branch Sep 28, 2017

hone added a commit to hone/rails that referenced this pull request Mar 1, 2018

Implement PDF Previewer Using Poppler
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment