Closed Bug 1180225 Opened 4 years ago Closed 4 years ago

skia::ConvolveHorizontally_SSE2 leaks uninitialised pixel values

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jseward, Assigned: jrmuizel)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files, 2 obsolete files)

These eventually re-surface at mozilla::image::imgFrame::Optimize()
as (presumably) the generated image(s) are checked to see if they
are all one colour.
Attached file Valgrind complaint
Generated on x86_64-linux, when loading
http://tangerine-tycoon.wikia.com/wiki/Tangerine_Tycoon_Wiki
A bit of digging shows this happens when the filter_length is not
divisible by 4, that is, this bit

    int r = filter_length & 3;
    if (r) {
      memcpy(&buffer, row_to_filter, r * 4);

Problem is that |buffer| contains 4 elements, all of which make it
to imgFrame::Optimize() (presumably), but the memcpy doesn't initialise
all of them.
Whiteboard: [gfx-noted]
Attached patch A possible fixSplinter Review
This zeroes out the otherwise-uninitialised 1, 2 or 3 vector elements
in |buffer|.

It looks as if ConvolveVertically_SSE2_impl, further down, has the same
problem, so this patch fixes that too.
Attachment #8629881 - Flags: review?(jmuizelaar)
So this was introduced by bug 1054510 making changes to the upstream code that I wasn't aware of. I'm going to investigate whether we should just do what upstream is doing instead.
Jeff, is there any progress on this?  Anything I can do to help move it along?
I tried switching to something closer to upstream and ran into some address sanitizer problems.

I've attached the WIP patch
It looks like there might be a bug in the upstream that's causing this. I'll continue to investigate.
Giving the convolver some extra room helps things.
Attachment #8637852 - Attachment is obsolete: true
(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> It's still broken:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=daf9a8f6b357

Wrong patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d21c9de8f24 will hopefully work better.
Attachment #8629881 - Flags: review?(jmuizelaar)
This adds 15 bytes of padding to the rows so that we don't have to worry about reading past the bounds and allows us to use a version of the convolver that's much closer to upstream.

Seth you're welcome to review the whole change but I'm mostly interested in the Downscaler.cpp changes.
Attachment #8646628 - Attachment is obsolete: true
Attachment #8650588 - Flags: review?(seth)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #12)
> Created attachment 8650588 [details] [diff] [review]
> Make convolver more like upstream

From my point of view, looks good.  I can no longer reproduce the V complaint
with this in place.
Comment on attachment 8650588 [details] [diff] [review]
Make convolver more like upstream

Review of attachment 8650588 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. (I didn't look closely at the convolver code changes; presumably taking us closer to upstream is also taking us in the direction of correctness.)

It'd be good to test locally on a page with scaled JPEGs (imgur.com is good for this), just to sanity check that there's no obvious bustage. For now we only have a very small number of tests that run with HQ scaling enabled, and when HQ scaling is disabled we also don't use DDD. I'd like to eventually turn on DDD for all tests, but additional work is needed to avoid the kinds of races that HQ scaling is subject to.
Attachment #8650588 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/a677282b7646
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Depends on: 1206744
You need to log in before you can comment on or make changes to this bug.