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

Add support for Raw files previews #14878

Closed
wants to merge 2 commits into from
Closed

Add support for Raw files previews #14878

wants to merge 2 commits into from

Conversation

oparoz
Copy link
Contributor

@oparoz oparoz commented Mar 13, 2015

Replaces #13652

Solution for

#13650
owncloud-archive/apps#439
owncloud-archive/apps#1113

Including media type migration and tests

Supported formats:

  • Sony Alpha RAW .arw
  • Canon RAW 2 .cr2
  • Digital Camera Raw .dcr
  • Digital Negative .dng
  • EPSON RAW .erf
  • Intelligent Image Quality RAW .iiq
  • Kodak DC40/DC50 RAW .kdc
  • Kodak DC25 RAW .k25
  • Nikon RAW .nef
  • Mamiya RAW .mef
  • Olympus RAW .orf
  • Panasonic RAW .rw2
  • PENTAX RAW .pef
  • Fujifilm RAW .raf
  • Sony RAW File .srf .sr2
  • SIGMA RAW .xrf

Requirements

@ghost
Copy link

ghost commented Mar 13, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/10443/
Test PASSed.

@DeepDiver1975 DeepDiver1975 added this to the 8.1-current milestone Mar 16, 2015
@DeepDiver1975
Copy link
Member

👍 looks good

@DeepDiver1975
Copy link
Member

@nickvergessen @MorrisJobke mind to review? THX

@nickvergessen
Copy link
Contributor

Wait for #14857 which just needs a rebase due to a merge conflict and then put this in an app?

@oparoz
Copy link
Contributor Author

oparoz commented Mar 16, 2015

@nickvergessen - OK, I can do it via an app, once the other ImageMagick providers have been moved to an app, so that I can use that as an example.

@DeepDiver1975
Copy link
Member

and then put this in an app?

well - mime changes have to go to core - moving the preview impl to an app - yes

@oparoz
Copy link
Contributor Author

oparoz commented Mar 16, 2015

@DeepDiver1975 - #14927 is for the new media types

namespace OC\Preview;

// Supports many file extensions linked to RAW
class Raw extends Bitmap {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an app now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find the other providers in apps and all the current providers are still in https://github.com/owncloud/core/tree/master/lib/private/preview

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't matter, its a new provider so it should not be here.
This is just a "memory", so we don't merge it like that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, waiting for #14803 , with one week to go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of waiting, send a patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to be the guinea pig on this. It was decided that providers need to exist as apps and someone must have a clear vision on how this is going to work, without using any private APIs and without breaking existing apps.
There needs to be at least one provider moved to an app, to make sure this actually works and to serve as a guide for app devs.

Good examples would be a sub-class of the bitmap or office providers since each requires 2 apps (base and sub) which need to communicate with each other using undefined methods (inter-app dependencies).
The movie provider which relies on core classes would be a good case for later as it might highlight missing public methods required to get it to run.

@DeepDiver1975
Copy link
Member

@oparoz @nickvergessen what's the status on this? THX

@oparoz
Copy link
Contributor Author

oparoz commented Apr 9, 2015

My take is that preview providers as apps is an untested concept. There is no process, no documentation.
For this particular provider, the parent class would be in core and in the private domain.
If the bitmap class is converted into an app, there is no definition on how to manage inter-app dependencies for all the other apps which would extend that one. The alternative being to have a "Bitmap" app which would be extended to support more formats via PRs.

So, from my point of view, there is no harm in adding this provider as the last one and then move everything out for 8.2. It's going to have to be done anyway.
Starting with this one and then modifying it several times to make it work like the rest of them seems like a waste of time.

@nickvergessen probably has a better idea of how this will all work in the future.

@DeepDiver1975
Copy link
Member

So, from my point of view, there is no harm in adding this provider as the last one and then move everything out for 8.2. It's going to have to be done anyway.

sounds reasonable to be

@oparoz can I ask you to rebase this branch please - THX
@nickvergessen any objections with respect to the suggested approach? THX

@nickvergessen
Copy link
Contributor

fine by me

@oparoz
Copy link
Contributor Author

oparoz commented Apr 10, 2015

OK, I'll rebase and we can talk about the next steps here:
#14803

If we're confident that everything will be in place for 8.2, this feature in core should be marked as depreciated. That way we can progressively create replacement apps and delete the whole thing in core for the launch of 8.2

Solutions for
#13650
owncloud-archive/apps#439
owncloud-archive/apps#1113

Supported formats:
* Sony Alpha RAW .arw
* Canon RAW 2 .cr2
* Digital Camera Raw .dcr
* Digital Negative .dng
* EPSON RAW .erf
* Intelligent Image Quality RAW .iiq
* Kodak DC40/DC50 RAW .kdc
* Kodak DC25 RAW .k25
* Nikon RAW .nef
* Mamiya RAW .mef
* Olympus RAW .orf
* Panasonic RAW .rw2
* PENTAX RAW .pef
* Fujifilm RAW .raf
* Sony RAW File .srf .sr2
* SIGMA RAW .xrf
@MorrisJobke
Copy link
Contributor

@oparoz I uploaded an image from the linked site and one of my raw images:
Both get this:

{"reqId":"hJYA0jx8MQeSEALUxgI0","remoteAddr":"127.0.0.1","app":"core","message":"Generating preview for \"\/\/RAW_LEICA_M8.DNG\" with \"OC\\Preview\\Raw\"","level":0,"time":"2015-04-12T08:17:17+00:00","method":"GET","url":"\/master\/index.php\/core\/preview.png?file=%2F%2FRAW_LEICA_M8.DNG&c=863a945b828fefdca8e7fb6d40889e06&x=36&y=36&forceIcon=0"}
{"reqId":"hJYA0jx8MQeSEALUxgI0","remoteAddr":"127.0.0.1","app":"core","message":"ImageMagick says: unable to open image `\/tmp\/magick-2219lYgfWFmGAjE9.ppm': No such file or directory @ error\/blob.c\/OpenBlob\/2675","level":3,"time":"2015-04-12T08:17:17+00:00","method":"GET","url":"\/master\/index.php\/core\/preview.png?file=%2F%2FRAW_LEICA_M8.DNG&c=863a945b828fefdca8e7fb6d40889e06&x=36&y=36&forceIcon=0"}

@oparoz
Copy link
Contributor Author

oparoz commented Apr 12, 2015

I suspect you haven't installed ufraw-batch so that ImageMagick can convert Raw images to PNG.

@MorrisJobke
Copy link
Contributor

I suspect you haven't installed ufraw-batch so that ImageMagick can convert Raw images to PNG.

Is this somewhere written? Otherwise we need a hint that this is needed. I would like to have it in the sample config too.

@oparoz
Copy link
Contributor Author

oparoz commented Apr 12, 2015

I think that in general, configuring ImageMagick is out of scope of a PR, but since you're the 2nd person running into the issue, a little hint in the sample config wouldn't hurt.

Would that be enough you think?

@nickvergessen
Copy link
Contributor

Well if we can check that programmatically, we should do it, where we check for the imagick plugin

@oparoz
Copy link
Contributor Author

oparoz commented Apr 13, 2015

I could add a check to the class

\OC_Helper::canExecute("ufraw-batch")

It should work for most people, but will hinder the move to the apps "domain", because there is no public method equivalent.

@scrutinizer-notifier
Copy link

The inspection completed: 4338 Issues, 132 Patches

@ghost
Copy link

ghost commented Apr 16, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/11589/
🚀 Test PASSed.🚀
chuck

@oparoz
Copy link
Contributor Author

oparoz commented Apr 20, 2015

The comment has been added, as well as a simple check for the helper binary.
@georgehrke - Could you see if this works on your setup?

@PVince81
Copy link
Contributor

Moving this enhancement to 8.2

@PVince81 PVince81 modified the milestones: 8.2-next, 8.1-current Apr 23, 2015
@oparoz
Copy link
Contributor Author

oparoz commented Apr 23, 2015

@PVince81 - There is no reason to push this back. It works and is ready to be merged.

@PVince81
Copy link
Contributor

The reason is feature freeze. Only bugfixes are allowed.
From what I understand this is an addition/enhancement/new feature, so it will have to wait for 8.2.

@oparoz
Copy link
Contributor Author

oparoz commented Apr 23, 2015

This should not be considered a last minute enhancement. The original PR has been created 3 months ago, long before the feature freeze deadline. Even the last revision is from 5-6 weeks ago. It was added to the list of planned features for 8.1 and it doesn't touch any vital system component unlike some of the bug fixing which has been happening these past weeks.

@oparoz
Copy link
Contributor Author

oparoz commented Apr 23, 2015

Never mind. I've just re-read your definition of Feature Freeze and PRs have to be merged before the deadline. I should have hassled people ;). As providers aren't accepted beyond 8.1, I'll just delete this PR.

@oparoz oparoz closed this Apr 23, 2015
@oparoz oparoz removed this from the 8.2-next milestone Apr 23, 2015
@oparoz oparoz deleted the raw-picture-preview branch April 23, 2015 22:15
@enoch85
Copy link
Member

enoch85 commented May 26, 2015

What happened with this? Will it be included in coming releases?

@oparoz
Copy link
Contributor Author

oparoz commented May 26, 2015

Unfortunately, no, but this can be added via apps.

@enoch85
Copy link
Member

enoch85 commented May 26, 2015

@oparoz Asking because I have some requests here: http://www.sweclockers.com/forum/trad/1212245-stora-owncloud-traden

Any plans on including this soon?

@oparoz
Copy link
Contributor Author

oparoz commented May 26, 2015

It will never be part of the core, but if you look at the code, you can see that it shouldn't be too difficult for an apps developer to build.
Maybe use BountySource?

@mbewley
Copy link

mbewley commented Oct 19, 2015

Out of interest, where is this at? I'm trying to figure out whether preview of RAW images (specifically ORF) is possible in owncloud - it would be very handy for my workflow!

@oparoz
Copy link
Contributor Author

oparoz commented Oct 19, 2015

@mbewley - It is possible, but it requires the development of an app. Since so few people are interested, I'm not sure I'm going to sell one (thousands to set up vs hundreds in subscriptions).
It seems BountySource doesn't offer fundraisers any more, but I'm sure there are alternative systems which could be used to incite developers to come up with a solution.
Could also be a good Summer of Code project for 2016.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants