---- Reported by shotwell-maint@gnome.bugs 2010-08-04 02:03:00 -0700 ----

Original Redmine bug id: 2356
Original URL: http://redmine.yorba.org/issues/2356
Searchable id: yorba-bug-2356
Original author: Josh Freeman
Original description:

This patch speeds up color transformations, which makes it easier to do color
correction: Images are redrawn faster when moving an adjustment slider, so the
visual feedback to the user is more responsive and precise, helping them find
the best adjustment value quicker.

The speedup will also help with the to-be-implemented contrast adjustment
that's expected to need an [comment:5:#2323 extra pass over the image data].

--

All but one of the transformations now use lookup tables; Saturation still
uses a matrix, but it's now fixed-point. (Saturation's still relatively slow
– adding a Sat adjustment will slow down the transformation set more than
all the other adjustments combined, so for best responsiveness, adjust the Sat
slider last.)

This patch changes the ColorTransformation classes, so it conflicts with my
earlier patch, Tint & Temperature improvements. The T&T patch is relatively
small, so I can rewrite & resubmit it if this one's accepted. (This patch's
Tint & Temperature algorithms match the current trunk.)

Hope this is useful – please let me know if there's any problems or
questions!



---- Additional Comments From shotwell-maint@gnome.bugs 2011-04-07 09:05:00 -0700 ----

### History

####

#1

Updated by Josh Freeman over 3 years ago

Duplicate ticket

####

#2

Updated by Jim Nelson over 3 years ago

  * **Status** changed from _Open_ to _Review_
  * **Assignee** changed from _Anonymous_ to _Josh Freeman_

This is an impressive patch. I am seeing a speedup in the color adjustments by
orders of magnitude. On an 8 megapixel image, the current trunk adjustment
needs (building Shotwell with the MEASURE_PIPELINE define) 0.481282s to
complete the transformation. With the patch, the same image is adjusted in
0.003373s. As the two most expensive operations in the pipeline are the scaled
load-and-decode and the color adjustment, effort in this area is appreciated.

One thing I'm noticing is that, on bright photos, if I adjust the Exposure
slider, the histogram becomes blocky or jagged, as some of the peaks and
valleys are not being smoothed. The histogram flashes as well sometimes. I
don't see this behavior in trunk (when there are peaks and valleys, they're
not quite as pronounced). Can you correct this?

There seems to be a slight color differences with side-by-side comparisons of
this patch versus trunk. I determined this by exporting the same photo from
patched and trunk versions. (Also by watching the photo come when double-
clicked; the old thumbnail is loaded first, and then the pipelined image is
painted on top of it. In Shotwell, thumbnails are always fully transformed.)
In particular, saturation seems to be bolder with the patch. This could be the
result of a number of factors, including (a) the algorithm is different (and
not merely computed faster), (b) rounding or precision issues, or © the
values stored in the database are interpreted differently between the two
algorithms. Can you suggest which of these (or other reasons) might cause
this? Changes in the color adjustment algorithm have other ramifications, and
the answer would determine what steps we would need to take.

Two of the members of the Shotwell team are on vacation this week, so we won't
be able to fully discuss and review this patch until they return (as well as
the tint & temperature patch). I do think this is all very promising.

####

#3

Updated by Jim Nelson over 3 years ago

I've attached two photos demonstrating the differences I mentioned.

####

#4

Updated by Jan-Christoph Borchardt over 3 years ago

The responsiveness and visual feedback of the adjustment sliders also came up
in the preliminary usability test.

Props for working on fixing it, tedge. :)

####

#5

Updated by Josh Freeman over 3 years ago

The new attached patch, shotwell_fast_color_transforms_fix1.diff, addresses a
few issues:

- The Saturation differences between the trunk & previous patch were due to a bug with in-place (same source & destination bitmap) matrix transformations: Each channel value of a pixel was transformed and written to the destination before calculating the remaining channel value(s). When the destination and source were the same, a pixel's 2nd & 3rd channel calculations would be incorrect, due to using the transformed values instead of the original ones.

- PixelTransformer's copy() method wasn't setting the value of the matrix transformation's identity flag, so it remained at the default (true). This caused previews/thumbnails to be generated without the Saturation transform.

- Fixed-point calculations now use a round-off value when converting back to integer.

####

#6

Updated by Josh Freeman about 3 years ago

Regarding the histogram flashing:

Transformations that expand or contract the color table will add gaps or
spikes to the histogram. The trunk implementation smoothes the raw histogram
data in order to remove these spikes & gaps. (The patched version does too,
since there's no changes in this part.)

Occasionally, the smoothing algorithm can leave spikes at the endpoints of a
transformed table even though a slightly different value for the transform
parameter leaves no spike. The endpoint spike can have a noticeable effect on
the scaling factor of the histogram (since the scale's based on the table's
maximum value – likely in this case to come from the spike).

If the user moves the Exposure slider between two values – one of which
produces a table with a spike, the other without – the significant change to
the scaling factor makes the histogram appear to jump (as a spiked table's
non-spike values are squashed to the bottom so the spike can fit on the
graph).

The reason this is more noticeable in the speedup patch is because the
histogram updates more rapidly when moving the slider. The faster updates
cause any jumps in the scaling factor to appear as a quick flash, as opposed
to the trunk implementation, where a single frame stays onscreen for longer,
so jumps are slower & less jarring. The trunk's fewer drawn frames also means
fewer chances to run across a frame with a scaling jump.

Regarding the histogram blockiness/jaggedness:

A transformation that compresses the color table, such as Exposure, will add
an increasing number of single-width spikes to the raw histogram as the effect
is increased (two or more of the old color table's values have to be combined
into a single value in order to fit in the compressed space of the new table).
As the number of spikes increases, distances between spikes grow smaller –
eventually two spikes are neighbors (no gap between them).

The trunk's smoothing algorithm only handles single-width spikes (the smoothed
value comes from averaging both neighboring values), so when two spikes are
neighbors, they both bring up the other's average enough to remain stuck out
in the output histogram. They show up as blocky, double-wide spikes – as
more of them appear, the formerly-smooth histogram quickly becomes jagged.

The fast smooth-to-jagged transition by the smoothing algorithm also makes it
hard to compare blockiness/jaggedness between the trunk and patch, because raw
histograms that are quite similar may end up looking very different after
processing.

Proposed solution:

The histogram is a tool for examining characteristics of an image –
smoothing the table just to make it look pleasant reduces its usefulness. Gaps
in the table are important to see because they can identify banding &
posterization, and spikes can show where tonal detail has been lost.
Additionally, the smoothing introduces flashing & blockiness issues.

I'd recommend removal of the histogram smoothing; instead, use the raw data to
draw the histogram. (Probably with some added tweaks to the calculation of the
scaling factor, in order to allow disproportional spikes to scale above the
top (truncating) so the rest of the histogram isn't squashed.)

####

#7

Updated by Josh Freeman about 3 years ago

  * **Assignee** changed from _Josh Freeman_ to _Jim Nelson_

I can submit a diff for the histogram if there's no objection to the smoothing
removal, but that probably belongs in a separate patch/ticket.

Reassigning to jim: I think my comments above address the issues you
mentioned, but please assign back if there's still issues or questions.

####

#8

Updated by Jim Nelson about 3 years ago

No problem, tedge. We're just getting through a burst of unexpected activity
and returning to 0.8 development. I'll look over the patch and talk with the
others to see how to proceed.

####

#9

Updated by Josh Freeman about 3 years ago

Patch v3 is an additional tweak to apply lookup transforms first instead of
matrix transforms (just Saturation) first.

The previous patch applies the Saturation transform first, so if Saturation's
turned all the way down and the user changes Tint or Temperature, the result
is a tinted monochrome image instead of a grayscale.

Also, to reduce roundoff error when applying both lookup & matrix transform
types, matrix transforms read from lookup tables with 32-bit fixed-point
values instead of 8-bit integer values.

####

#10

Updated by Josh Freeman about 3 years ago

(Just bumping the ticket's modification date, since attaching a file (patch
v4) doesn't appear to have updated it.)

Patch v4 has two more tweaks:

- RGBHistogramManipulator now sends signals during nub tracking so the Expansion transformation can draw updates while the mouse drags.

- The color adjustment sliders have a noticably faster response. (Moving a slider quickly would generate too many SliderAdjustmentCommands for GDK's paint() to be able to keep up, so SliderAdjustmentCommands are now throttled.)

####

#11

Updated by Adam Dingle about 3 years ago

  * **Priority** deleted (<strike>_High_</strike>)

Will not make 0.8, but still a strong candidate for 0.9. Dropping to medium.

####

#12

Updated by Adam Dingle almost 3 years ago

  * **Priority** set to _High_

####

#13

Updated by Adam Dingle over 2 years ago

  * **Assignee** changed from _Jim Nelson_ to _Anonymous_



--- Bug imported by chaz@yorba.org 2013-11-25 21:46 UTC  ---

This bug was previously known as _bug_ 2356 at http://redmine.yorba.org/show_bug.cgi?id=2356
Imported an attachment (id=261747)
Imported an attachment (id=261748)
Imported an attachment (id=261749)
Imported an attachment (id=261750)
Imported an attachment (id=261751)
Imported an attachment (id=261752)

Unknown Component 
   Using default product and component set in Parameters 
Unknown version " in product shotwell. 
   Setting version to "!unspecified".
Unknown milestone "unknown in product shotwell. 
   Setting to default milestone for this product, "---".
Setting qa contact to the default for this product.
   This bug either had no qa contact or an invalid one.
Resolution set on an open status.
   Dropping resolution