Bug 331690 - Artifacts in Emboss with Variable Depth filter.
Summary: Artifacts in Emboss with Variable Depth filter.
Status: RESOLVED FIXED
Alias: None
Product: krita
Classification: Unclassified
Component: Filters (show other bugs)
Version: git master (please specify the git hash!)
Platform: unspecified Other
: NOR normal
Target Milestone: ---
Assignee: Krita Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-03 06:39 UTC by Spencer Brown
Modified: 2015-01-15 12:06 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Spencer Brown 2014-03-03 06:39:33 UTC
Images showing the problem:

http://i.imgur.com/F08OFCV.jpg (Windows 64-bit 2.8 beta 3 d0d3125 on AMD FX-8320)

http://i.imgur.com/1ig9WlQ.png (Arch Linux 32-bit git master ff1af84 on Intel i3-2310m)

The artifacts seem to show up in any color space and representation, including 8-bit sRGB; I also get something similar (http://i.imgur.com/A5drAt3.png) on a filter that I'm working on that follows a similar code path to the variable-height emboss filter, i.e. in processImpl(KisPaintDeviceSP device, ...) get a KisSequentialIterator across the pixels, then for each pixel assign a new color with this call: device->colorSpace()->fromQColor(QColor(/something/), it.rawData())

Currently my custom filter should only make every pixel in the layer red and opaque. This is not the final intended behavior. I would greatly appreciate it if someone would recommend an alternative code path that I could use that does something similar.

I've posted a link to a tarball with my filter in it. Of course, you should read the code before you execute something arbitrary on your machine.  I have a hunch that if we can get the red filter working right, the emboss filter will work right too.

Reproducible: Always

Steps to Reproduce:
1. Get an image. A blank image will work fine.
2. Go to Filter menu->Emboss->Emboss with Variable Depth...
Actual Results:  
Bad artifacts, probably something low-level gone wrong.

Expected Results:  
Filter should be somewhat consistent with the emboss filters that use convolution kernels to produce a result.

Red filter source code: https://dl.dropboxusercontent.com/u/15437561/redfilter.tar.gz
Comment 1 Halla Rempt 2014-03-03 09:26:22 UTC
Yes. I see what you mean. I'm going to take a look at your code.
Comment 2 Halla Rempt 2014-03-03 18:19:50 UTC
Hm, well -- using KisSequentialIterator is correct -- I cannot reproduce artefacts with your filter (I woudnt have expected them, either). Your other option would be something like is done in kis_unsharp_filter, where a hline iterator is created and then we loop over the rows:

    KisHLineIteratorSP dstIt = device->createHLineIteratorNG(rect.x(), rect.y(), rect.width());

    quint8 *colors[2];
    colors[0] = new quint8[pixelSize];
    colors[1] = new quint8[pixelSize];

    for (int j = 0; j < rect.height(); j++) {
        do {
                 ...
        } while (dstIt->nextPixel());
        dstIt->nextRow();
    }
But that is actually less efficient -- though it might be necessary if you use more than one paint device at the same time.
Comment 3 Halla Rempt 2014-03-03 18:53:38 UTC
The big problem with the emboss filter is that it needs pixel information outside the rect that it gets; krita filters can be applied tile-wise, without the filter know anything about that. So if your filter needs access to pixels outside the rect it is allowed to change, it has to ask for that.

Apart from that, the ancient code Michael Thaler copied ten years ago from Digikam has more problems -- I see some overflows as well. Let's see what can be salvaged.
Comment 4 Spencer Brown 2014-03-03 19:46:55 UTC
Thanks, that helps a lot with what I'm working on. My filter doesn't make much sense without write access to all the pixels in the image, so I think I'll have to make sure it requests sequential access properly. After I've got it working and the patch submitted I may go back and see if I can improve performance by computing both passes in the Euclidean distance transform at once, then combining the results for each pass.
Comment 5 Halla Rempt 2014-03-03 19:54:04 UTC
If you add 

setSupportsThreading(false);

in the constructor, the filter will always get the whole image in one go
Comment 6 Halla Rempt 2014-03-03 20:18:37 UTC
Git commit d482cd2ec24db1f2dfe7ee23b5b35ab3117d3f1f by Boudewijn Rempt.
Committed on 03/03/2014 at 20:17.
Pushed by rempt into branch 'master'.

The emboss filter needs access to the whole image

Note: there still are some weird things, like color pixels showing up...

M  +1    -0    krita/plugins/filters/embossfilter/kis_emboss_filter.cpp

http://commits.kde.org/calligra/d482cd2ec24db1f2dfe7ee23b5b35ab3117d3f1f
Comment 7 Halla Rempt 2014-03-03 20:19:01 UTC
Git commit 6ccdc18bf2e6856e656b8ed59b63577961d005c1 by Boudewijn Rempt.
Committed on 03/03/2014 at 20:17.
Pushed by rempt into branch 'calligra/2.8'.

The emboss filter needs access to the whole image

Note: there still are some weird things, like color pixels showing up...

M  +1    -0    krita/plugins/filters/embossfilter/kis_emboss_filter.cpp

http://commits.kde.org/calligra/6ccdc18bf2e6856e656b8ed59b63577961d005c1