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

Applying bitmap max preview patch fails (8.0.3) #150

Closed
shaneonabike opened this issue May 20, 2015 · 19 comments
Closed

Applying bitmap max preview patch fails (8.0.3) #150

shaneonabike opened this issue May 20, 2015 · 19 comments
Assignees
Labels
Milestone

Comments

@shaneonabike
Copy link

Hey there

This is such a great addition to owncloud!!

On OwnCloud 8.0.3 (stable) I applied the suggested patches as follows:

  • max-preview.pull.13674.patch
  • bitmap-max-preview.pull.13635.patch

The first one went through properly but the second seems to have failed tragically... is there a new release of this patch (I tried to find a corresponding ticket for this just to download it myself but no dice)

 ::::::::::::::
lib/private/preview/bitmap.php.rej
::::::::::::::
--- lib/private/preview/bitmap.php.orig 2015-03-06 15:14:17.770952395 +0000
+++ lib/private/preview/bitmap.php  2015-03-06 15:02:38.803951481 +0000
@@ -9,31 +10,93 @@
 
 namespace OC\Preview;
 
+use Imagick;
+
+/**
+ * Creates a PNG preview using ImageMagick via the PECL extension
+ *
+ * @package OC\Preview
+ */
 abstract class Bitmap extends Provider {
+
+   /**
+    * @type array
+    */
+   private $maxDims;
+
    /**
     * {@inheritDoc}
     */
    public function getThumbnail($path, $maxX, $maxY, $scalingup, $fileview) {
+       $this->maxDims = [$maxX, $maxY];
+
        $tmpPath = $fileview->toTmpFile($path);
+       if (!$tmpPath) {
+           return false;
+       }
 
-       //create imagick object from bitmap or vector file
+       // Creates \Imagick object from bitmap or vector file
        try {
-           // Layer 0 contains either the bitmap or
-           // a flat representation of all vector layers
-           $bp = new \Imagick($tmpPath . '[0]');
-
-           $bp->setImageFormat('png');
+           $bp = $this->getResizedPreview($tmpPath);
        } catch (\Exception $e) {
-           \OC_Log::write('core', $e->getmessage(), \OC_Log::ERROR);
+           \OC_Log::write('core', 'ImageMagick says : ' . $e->getmessage()
, \OC_Log::ERROR);
            return false;
        }
 
        unlink($tmpPath);
 
        //new bitmap image object
-       $image = new \OC_Image($bp);
+       $image = new \OC_Image();
+       $image->loadFromData($bp);
        //check if image object is valid
        return $image->valid() ? $image : false;
    }
-}
 
+   /**
+    * Returns a preview of maxX times maxY dimensions in PNG format
+    *
+    *  * The default resolution is already 72dpi, no need to change it for a b
itmap output
+    *  * It's possible to have proper colour conversion using profileimage().
+    *  ICC profiles are here: http://www.color.org/srgbprofiles.xalter
+    *  * It's possible to Gamma-correct an image via gammaImage()
+    *
+    * @param string $tmpPath the location of the file to convert
+    *
+    * @return \Imagick
+    */
+   private function getResizedPreview($tmpPath) {
+       $bp = new Imagick();
+
+       // Layer 0 contains either the bitmap or a flat representation of all v
ector layers
+       $bp->readImage($tmpPath . '[0]');
+
+       $bp = $this->resize($bp);
+
+       $bp->setImageFormat('png');
+
+       return $bp;
+   }
+
+   /**
+    * Returns a resized \Imagick object
+    *
+    * If you want to know more on the various methods available to resize an
+    * image, check out this link : @link https://stackoverflow.com/questions/85173
04/what-the-difference-of-sample-resample-scale-resize-adaptive-resize-thumbnail-im
+    *
+    * @param \Imagick $bp
+    *
+    * @return \Imagick
+    */
+   private function resize($bp) {
+       list($maxX, $maxY) = $this->maxDims;
+       list($previewWidth, $previewHeight) = array_values($bp->getImageGeometr
y());
+
+       // We only need to resize a preview which doesn't fit in the maximum di
mensions
+       if ($previewWidth > $maxX || $previewHeight > $maxY) {
+           // TODO: LANCZOS is the default filter, CATROM could bring simi
lar results faster
+           $bp->resizeImage($maxX, $maxY, imagick::FILTER_LANCZOS, 1, true
);
+       }
+
+       return $bp;
+   }
+}
@setnes
Copy link
Contributor

setnes commented May 21, 2015

Here's the output when the patch fails. (I have the same issue.)

ownCloud 8.0.3
Gallery 8.0.11

# patch -p1 -l < apps/galleryplus/patches/bitmap-max-preview.pull.13635.patch
patching file lib/private/preview/bitmap.php
patch unexpectedly ends in middle of line
Hunk #2 FAILED at 10.
1 out of 2 hunks FAILED -- saving rejects to file lib/private/preview/bitmap.php.rej

@oparoz oparoz added the bug label May 21, 2015
@oparoz oparoz self-assigned this May 21, 2015
@oparoz oparoz added this to the 8.0.12 milestone May 21, 2015
@oparoz
Copy link
Contributor

oparoz commented May 21, 2015

Thanks for the report. I did fix it for myself, but forgot to push the change.

@shaneonabike
Copy link
Author

Let us know when it's rolled out and I'll grab the latest to test!!

Again thanks for this awesome addon!

On 15-05-21 06:13 AM, Olivier Paroz
  wrote:


  Thanks for the report. I did fix it for myself, but forgot to
    push the change.
  —
    Reply to this email directly or view
      it on GitHub.









-- 

    Shane Bill
    Web Development & Consultant



          E-mail
          shanebill@gmail.com



          Phone
          438-934-2624


          More information
          https://develop.beesonabike.com

@oparoz
Copy link
Contributor

oparoz commented May 21, 2015

I've just checked and didn't understand why I could apply it just fine and then realised it is already fixed in master, so just download it from there.

I'm waiting on 2 PRs before releasing 8.0.12 for 8.0.3+

@setnes
Copy link
Contributor

setnes commented May 21, 2015

This? https://raw.githubusercontent.com/owncloud/galleryplus/master/patches/bitmap-max-preview.pull.13635.patch

It is the same as what I got from 169116-galleryplus-8.0.11.zip. It doesn't work. Is there a newer patch we are not seeing?

@oparoz
Copy link
Contributor

oparoz commented May 21, 2015

Ah, you're right, silly me. I updated the raw patch...

@oparoz oparoz reopened this May 21, 2015
@oparoz
Copy link
Contributor

oparoz commented May 22, 2015

I can't reproduce...

I've downloaded the zip from master
https://github.com/owncloud/galleryplus/archive/master.zip

# patch --check -p1 -l < customapps/galleryplus/patches/bitmap-max-preview.pull.13635.patch
Patching file lib/private/preview/bitmap.php using Plan A...
Hunk #1 succeeded at 1.
Hunk #2 succeeded at 10.
done
# patch --check -p1 -l < customapps/galleryplus/patches/max-preview.pull.13674.patch
Patching file config/config.sample.php using Plan A...
Hunk #1 succeeded at 609 (offset 1 line).
Patching file lib/private/preview.php using Plan A...
Hunk #1 succeeded at 2.
Hunk #2 succeeded at 83 (offset 1 line).
Hunk #3 succeeded at 349 (offset 1 line).
Hunk #4 succeeded at 359 (offset 1 line).
Hunk #5 succeeded at 372 (offset 1 line).
Hunk #6 succeeded at 399 (offset 1 line).
Hunk #7 succeeded at 421 (offset 1 line).
Hunk #8 succeeded at 429 (offset 1 line).
Hunk #9 succeeded at 487 (offset 1 line).
Hunk #10 succeeded at 501 (offset 1 line).
Hunk #11 succeeded at 545 (offset 1 line).
Hunk #12 succeeded at 615 (offset 1 line).
Hunk #13 succeeded at 648 (offset 1 line).
Hunk #14 succeeded at 667 (offset 1 line).
Hunk #15 succeeded at 700 (offset 1 line).
Hunk #16 succeeded at 1103 (offset 1 line).
done

@setnes
Copy link
Contributor

setnes commented May 22, 2015

Weird. The file from that master.zip matches the patch file I have. Perhaps our bitmap.php files are different. This is what I got from ownCloud 8.0.3.

<?php
/**
 * Copyright (c) 2013-2014 Georg Ehrke georg@ownCloud.com
 * This file is licensed under the Affero General Public License version 3 or
 * later.
 * See the COPYING-README file.
 */

namespace OC\Preview;

abstract class Bitmap extends Provider {
    /**
     * {@inheritDoc}
     */
    public function getThumbnail($path, $maxX, $maxY, $scalingup, $fileview) {
        $tmpPath = $fileview->toTmpFile($path);

        //create imagick object from bitmap or vector file
        try {
            // Layer 0 contains either the bitmap or
            // a flat representation of all vector layers
            $bp = new \Imagick($tmpPath . '[0]');

            $bp->setImageFormat('png');
        } catch (\Exception $e) {
            \OC_Log::write('core', $e->getmessage(), \OC_Log::ERROR);
            return false;
        }

        unlink($tmpPath);

        //new bitmap image object
        $image = new \OC_Image($bp);
        //check if image object is valid
        return $image->valid() ? $image : false;
    }
}

@setnes
Copy link
Contributor

setnes commented May 22, 2015

Would some checksums be helpful? Do these match what you get with md5sum?

1c1f51ded41dab8956dd247638e0af01  lib/private/preview/bitmap.php
9f98eb570cb7341f965823c8ce4a0da9  apps/galleryplus/patches/bitmap-max-preview.pull.13635.patch

@oparoz
Copy link
Contributor

oparoz commented May 22, 2015

I have exactly the same md5...

@oparoz
Copy link
Contributor

oparoz commented May 22, 2015

On what environment are you?

It must be a line-ending issue.

You could try running dos2unix to see if it improves the situation

And also check that lines don't break in the patch

@setnes
Copy link
Contributor

setnes commented May 22, 2015

I am on Linux (Gentoo). More importantly though, I am using GNU patch 2.7.5. Different end of line characters should result in different MD5 checksums.

@oparoz
Copy link
Contributor

oparoz commented May 22, 2015

I'm on FreeBSD. I know some of the options are different.
You could try replacing -l with -w

@shaneonabike
Copy link
Author

So for Redmine 3.0.1 I can just download the latest and test this out? Just want to be sure before I explode stuff ;)

I'm on Ubuntu 14 btw

@oparoz
Copy link
Contributor

oparoz commented May 22, 2015

For oC 8.0.3 you mean? You can download master, yes.

@shaneonabike
Copy link
Author

Haa haa whoops someone needed more coffee.. cool I'll give it a try for oCloud

@shaneonabike
Copy link
Author

Cool! Master worked perfectly for me thanks heaps.. AND I really like the new sort types (a-z and date) nice work!

@setnes
Copy link
Contributor

setnes commented May 24, 2015

I figured this out. The last line in the patch file does not contain a newline character. This causes the patch to fail with GNU patch and evidently does not cause it to fail with the version of patch included with FreeBSD. Fixing the end of line character at the end of the file would make this patch more compatible with other systems.

So, this was an end-of-line issue, but there were no windows carriage return characters involved.

http://unix.stackexchange.com/questions/18743/whats-the-point-in-adding-a-new-line-to-the-end-of-a-file

@oparoz
Copy link
Contributor

oparoz commented May 24, 2015

You're right, it's missing and I'll add it. It's a mistake, ownClouders are pretty strict about these sort of things and that confirms why it's important...

@oparoz oparoz reopened this May 24, 2015
@oparoz oparoz closed this as completed May 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants