Merge lp:~victor-mireyev/simple-scan/484616 into lp:~simple-scan-team/simple-scan/trunk

Proposed by Victor Mireyev on 2013-06-04
Status: Rejected
Rejected by: Robert Ancell on 2013-06-06
Proposed branch: lp:~victor-mireyev/simple-scan/484616
Merge into: lp:~simple-scan-team/simple-scan/trunk
Diff against target: 98 lines (+28/-10)
3 files modified
src/book.vala (+21/-6)
src/jpeglib.vapi (+5/-0)
src/page.vala (+2/-4)
To merge this branch: bzr merge lp:~victor-mireyev/simple-scan/484616
Reviewer Review Type Date Requested Status
Robert Ancell 2013-06-04 Disapprove on 2013-06-06
Review via email: mp+167359@code.launchpad.net

This proposal supersedes a proposal from 2013-05-29.

Description of the change

Save DPI with JPEG

To post a comment you must log in.
Robert Ancell (robert-ancell) wrote : Posted in a previous version of this proposal

Can you remove the JpegWriter class back to how it was? It doesn't seem to add anything and it makes it harder to review.

Please correct the whitespace before the '(' in the use of functions.

Other than that, looks good.

review: Needs Fixing
Robert Ancell (robert-ancell) wrote : Posted in a previous version of this proposal

Is this an enum? Can you put the enum values into the .vapi if so?
+ info.density_unit = 1;

Victor Mireyev (greymouse2005) wrote : Posted in a previous version of this proposal

As you can see, there are three important changes:
1) optional density parameter is added
2) n_written parameter is removed as it's trivially obtained via jpeg_data.length
3) Quality and density code is added
92+ info.set_quality (90);
93 + if (density != null)
94 + {
95 + info.density_unit = 1;
96 + info.X_density = (uint16) density;
97 + info.Y_density = (uint16) density;
98 + }

There are two reason for existance of JpegWriter class.

1) JPEG compression code doesn't use any nonstatic methods or properties of the Book class.
2) As JPEG compression code is used in both Book and Page classes, it shouldn't be duplicated.

Do you propose make compress_jpeg method static?

Robert Ancell (robert-ancell) wrote :

Hi,

Sorry, I missed with why you changed compress_jpeg - if the code is common to both page.vala and book.vala it should go into it's own module (e.g. jpeg.vala).

However, while this bug does fix the DPI not being set it does mean we can't use the ICC profile support without re-implementing that too (it was commented out but it is supported in gdk-pixbuf). I meant to make a patch to gdk-pixbuf ages ago but I forgot - I've made this now for DPI [1].

I think that once the DPI patch is accepted to gdk-pixbuf we should just use that - it will be simpler for simple-scan without the jpeg code being accessed directly.

Sorry again, I should have picked this up earlier.

[1] https://bugzilla.gnome.org/show_bug.cgi?id=701622

review: Disapprove
Victor Mireyev (greymouse2005) wrote :

Thanks you so much for fixing this issue and sharing common code with gdk-pixbuf!

Unmerged revisions

598. By Victor Mireyev on 2013-05-30

Remove JpegWriter. New DensityUnit enumeration

597. By Victor Mireyev on 2012-09-03

Save DPI with JPEG.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/book.vala'
2--- src/book.vala 2013-05-27 22:18:27 +0000
3+++ src/book.vala 2013-06-04 18:10:37 +0000
4@@ -9,6 +9,13 @@
5 * license.
6 */
7
8+enum DensityUnit
9+{
10+ NO_UNITS,
11+ PIXELS_PER_INCH,
12+ PIXELS_PER_CENTIMETER
13+}
14+
15 public class Book
16 {
17 private List<Page> pages;
18@@ -183,7 +190,7 @@
19 private static bool jpeg_empty_cb (JPEG.Compress info) { return true; }
20 private static void jpeg_term_cb (JPEG.Compress info) {}
21
22- private uint8[] compress_jpeg (Gdk.Pixbuf image, out size_t n_written)
23+ public static uint8[] compress_jpeg (Gdk.Pixbuf image, int? density = null)
24 {
25 var info = JPEG.Compress ();
26 var jerr = JPEG.ErrorManager ();
27@@ -198,6 +205,15 @@
28 info.in_color_space = JPEG.ColorSpace.RGB; /* TODO: JCS_GRAYSCALE? */
29 info.set_defaults ();
30
31+ info.set_quality (90);
32+ if (density != null)
33+ {
34+ uint8 unit = DensityUnit.PIXELS_PER_INCH;
35+ info.density_unit = unit;
36+ info.X_density = (uint16) density;
37+ info.Y_density = (uint16) density;
38+ }
39+
40 var max_length = info.image_width * info.image_height * info.input_components;
41 var data = new uint8[max_length];
42 dest_mgr.next_output_byte = data;
43@@ -216,8 +232,8 @@
44 info.write_scanlines (row, 1);
45 }
46 info.finish_compress ();
47- n_written = max_length - dest_mgr.free_in_buffer;
48- data.resize ((int) n_written);
49+ int n_written = max_length - dest_mgr.free_in_buffer;
50+ data.resize (n_written);
51
52 return data;
53 }
54@@ -414,9 +430,8 @@
55 /* Try if JPEG compression is better */
56 if (depth > 1)
57 {
58- size_t jpeg_length;
59- var jpeg_data = compress_jpeg (image, out jpeg_length);
60- if (jpeg_length < compressed_data.length)
61+ var jpeg_data = compress_jpeg (image);
62+ if (jpeg_data.length < compressed_data.length)
63 {
64 filter = "DCTDecode";
65 data = jpeg_data;
66
67=== modified file 'src/jpeglib.vapi'
68--- src/jpeglib.vapi 2011-06-12 04:55:11 +0000
69+++ src/jpeglib.vapi 2013-06-04 18:10:37 +0000
70@@ -22,6 +22,11 @@
71 public int input_components;
72 public ColorSpace in_color_space;
73 public ErrorManager* err;
74+
75+ public uint8 density_unit;
76+ public uint16 X_density;
77+ public uint16 Y_density;
78+ public void set_quality (int quality, bool force_baseline = true);
79
80 public void create_compress ();
81 public void set_defaults ();
82
83=== modified file 'src/page.vala'
84--- src/page.vala 2013-05-19 21:55:27 +0000
85+++ src/page.vala 2013-06-04 18:10:37 +0000
86@@ -704,10 +704,8 @@
87
88 if (strcmp (type, "jpeg") == 0)
89 {
90- /* ICC profile is awaiting review in gtk2+ bugzilla */
91- string[] keys = { "quality", /* "icc-profile", */ null };
92- string[] values = { "90", /* icc_profile_data, */ null };
93- writer.save (image, "jpeg", keys, values);
94+ var data = Book.compress_jpeg (image, this.get_dpi ());
95+ stream.write_all (data, null, null);
96 }
97 else if (strcmp (type, "png") == 0)
98 {

Subscribers

People subscribed via source and target branches