| Summary: | test/cairo-test-suite reports wrong max diff comparing different opaque images | ||
|---|---|---|---|
| Product: | cairo | Reporter: | Massimo <sixtysix> |
| Component: | general | Assignee: | Chris Wilson <chris> |
| Status: | RESOLVED MOVED | QA Contact: | cairo-bugs mailing list <cairo-bugs> |
| Severity: | normal | ||
| Priority: | medium | ||
| Version: | unspecified | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: | patch | ||
|
Description
Massimo
2014-09-18 14:16:04 UTC
Created attachment 106538 [details] [review] patch Running DISPLAY=:2 time -p make -s test TARGETS=image,xlib,xcb without/with the attached patch on my laptop the last lines of test/cairo-test-suite.log change from: 322 Passed, 203 Failed [1 crashed, 8 expected], 29 Skipped Preamble: 1 failed - create-from-png image (argb32): 1 crashed! - gl-surface-source image (argb32): 41 failed - clip-operator extended-blend-alpha exten... image (rgb24): 1 crashed! - gl-surface-source image (rgb24): 49 failed - clip-operator extended-blend extended-ble... xlib (argb32): 1 crashed! - gl-surface-source xlib (argb32): 104 failed - aliasing arc-looping-dash bug-spline bug... xlib (rgb24): 1 crashed! - gl-surface-source xlib (rgb24): 142 failed - aliasing arc-looping-dash bug-spline bug-... xlib-window (rgb24): 1 crashed! - gl-surface-source xlib-window (rgb24): 140 failed - aliasing arc-looping-dash bug-spli... xlib-render-0_0 (rgb24): 1 crashed! - gl-surface-source xlib-render-0_0 (rgb24): 73 failed - caps-sub-paths clear-source cli... xlib-fallback (rgb24): 1 crashed! - gl-surface-source xlib-fallback (rgb24): 113 failed - bug-51910 bug-seams caps caps-jo... xcb (argb32): 1 crashed! - gl-surface-source xcb (argb32): 50 failed - bug-spline hatchings huge-radial operator-... xcb (rgb24): 1 crashed! - gl-surface-source xcb (rgb24): 99 failed - arc-looping-dash bug-spline bug-51910 bug-s... xcb-window (rgb24): 1 crashed! - gl-surface-source xcb-window (rgb24): 97 failed - arc-looping-dash bug-spline bug-5191... xcb-window& (rgb24): 1 crashed! - gl-surface-source xcb-window& (rgb24): 98 failed - arc-looping-dash bug-spline bug-519... xcb-render-0_0 (argb32): 1 crashed! - gl-surface-source xcb-render-0_0 (argb32): 55 failed - bug-seams clip-disjoint-hatchin... xcb-render-0_0 (rgb24): 1 crashed! - gl-surface-source xcb-render-0_0 (rgb24): 66 failed - bug-seams clip-disjoint-hatching... xcb-fallback (rgb24): 1 crashed! - gl-surface-source xcb-fallback (rgb24): 50 failed - bug-seams clip-operator extended-b... to: 365 Passed, 160 Failed [1 crashed, 8 expected], 29 Skipped Preamble: 1 failed - create-from-png image (argb32): 1 crashed! - gl-surface-source image (argb32): 41 failed - clip-operator extended-blend-alpha exten... image (rgb24): 1 crashed! - gl-surface-source image (rgb24): 31 failed - extended-blend-mask extended-blend-alpha-... xlib (argb32): 1 crashed! - gl-surface-source xlib (argb32): 104 failed - aliasing arc-looping-dash bug-spline bug... xlib (rgb24): 1 crashed! - gl-surface-source xlib (rgb24): 99 failed - aliasing arc-looping-dash bug-spline bug-e... xlib-window (rgb24): 1 crashed! - gl-surface-source xlib-window (rgb24): 97 failed - aliasing arc-looping-dash bug-splin... xlib-render-0_0 (rgb24): 1 crashed! - gl-surface-source xlib-render-0_0 (rgb24): 59 failed - clip-operator clip-push-group c... xlib-fallback (rgb24): 1 crashed! - gl-surface-source xlib-fallback (rgb24): 50 failed - bug-seams extended-blend-mask ext... xcb (argb32): 1 crashed! - gl-surface-source xcb (argb32): 50 failed - bug-spline hatchings huge-radial operator-... xcb (rgb24): 1 crashed! - gl-surface-source xcb (rgb24): 45 failed - bug-spline hatchings huge-radial overlappin... xcb-window (rgb24): 1 crashed! - gl-surface-source xcb-window (rgb24): 44 failed - bug-spline hatchings huge-radial ove... xcb-window& (rgb24): 1 crashed! - gl-surface-source xcb-window& (rgb24): 44 failed - bug-spline hatchings huge-radial ov... xcb-render-0_0 (argb32): 1 crashed! - gl-surface-source xcb-render-0_0 (argb32): 55 failed - bug-seams clip-disjoint-hatchin... xcb-render-0_0 (rgb24): 1 crashed! - gl-surface-source xcb-render-0_0 (rgb24): 49 failed - bug-seams clip-disjoint-hatching... xcb-fallback (rgb24): 1 crashed! - gl-surface-source xcb-fallback (rgb24): 32 failed - bug-seams extended-blend-mask exte... Isn't the bug here really in the code that compares the images?
You are blaming test/buffer-diff.c line 80 which is:
if ((row_a[x] & mask) != (row_b[x] & mask))
However, this mask value is set by the caller to either 0xffffffff or 0x00ffffff, depending on if the image has an alpha channel. In other words, this code correctly ignores the unused byte in RGB24 images.
The bug really is in test/pdiff/pdiff.c, function pdiff_compare(). This function does NOT do something like "cairo_surface_get_content (surface_a) & CAIRO_CONTENT_ALPHA ? 0xffffffff : 0x00ffffff" and treats all images as ARGB32.
This functions uses _get_red() etc to extract colors from the image:
177 static uint32_t
178 _get_pixel (const uint32_t *data, int i)
179 {
180 return data[i];
181 }
182
183 static unsigned char
184 _get_red (const uint32_t *data, int i)
185 {
186 uint32_t pixel;
187 uint8_t alpha;
188
189 pixel = _get_pixel (data, i);
190 alpha = (pixel & 0xff000000) >> 24;
191 if (alpha == 0)
192 return 0;
193 else
194 return (((pixel & 0x00ff0000) >> 16) * 255 + alpha / 2) / alpha;
195 }
So the real bug is in this code. No idea how it can be easily fixed. An ugly work around would be to copy the images in case they aren't format ARGB32 yet via cairo so that cairo makes sure the alpha channel is set to 0xff....?
(In reply to Uli Schlachter from comment #2) > Isn't the bug here really in the code that compares the images? > > You are blaming test/buffer-diff.c line 80 which is: > > if ((row_a[x] & mask) != (row_b[x] & mask)) > I blamed test/buffer-diff.c because when that condition is TRUE, the maximum diff computed includes also the non-alpha channel: http://cgit.freedesktop.org/cairo/tree/test/buffer-diff.c#n85 So my first observation was reading a test log in which two identical images (argb32 and rgb24) had reported two max diff values compared to the same reference. yes, there are also other bugs in test/pdiff (In reply to Massimo from comment #3) > (In reply to Uli Schlachter from comment #2) > > Isn't the bug here really in the code that compares the images? > > > > You are blaming test/buffer-diff.c line 80 which is: > > > > if ((row_a[x] & mask) != (row_b[x] & mask)) > > > > I blamed test/buffer-diff.c because when that condition is > TRUE, the maximum diff computed includes also the non-alpha > channel: > > http://cgit.freedesktop.org/cairo/tree/test/buffer-diff.c#n85 > > So my first observation was reading a test log in which > two identical images (argb32 and rgb24) had reported two > max diff values compared to the same reference. Ah, I missed that part. From a quick look, I'd fix that by changing these two lines: int value_a = (row_a[x] >> (channel*8)) & 0xff; int value_b = (row_b[x] >> (channel*8)) & 0xff; into int value_a = ((row_a[x] & mask) >> (channel*8)) & 0xff; Oh and line 101 needs to be changed as well, something like "&& mask != 0x00ffffff". Here it seems simpler to pass a cairo_format_t or cairo_bool_t into buffer_diff_core() and calculate the mask based on that. > yes, there are also other bugs in test/pdiff I'd rather fix these problems in pdiff instead of somewhere in the code producing these images. In theory, these two code paths are independent from each other and pdiff could be used from something else than just the test suite. (In reply to Uli Schlachter from comment #4) > > I'd rather fix these problems in pdiff instead of somewhere in the code > producing these images. In theory, these two code paths are independent from > each other and pdiff could be used from something else than just the test > suite. Agreed, I opted to fix the producer because a failing test compares an image with many references, but is produced only once, so with many failures in the test-suite this extra work is not repeated many times. OTOH to speed up the execution of the test suite probably it could be possible to save with the reference also its md5sum, so that for the exact comparison one computes the md5sum of the output and compares it against many references with only one image loop. -- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/cairo/cairo/issues/25. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.