Bug 49523

Summary: Splash::arbitraryTransformImage causes bogus memory allocation size
Product: poppler Reporter: Thomas Freitag <Thomas.Freitag>
Component: splash backendAssignee: poppler-bugs <poppler-bugs>
Status: RESOLVED FIXED QA Contact:
Severity: critical    
Priority: medium CC: fabiodurso, kovid, mpsuzuki, ralph
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Avoid bogus memory allocation size
Avoid bogus memory allocation size
Avoid bogus memory allocation size
Avoid bogus memory allocation size

Description Thomas Freitag 2012-05-05 02:15:39 UTC
The calculation of scaledWidth and scaledHeight in Splash::arbitraryTransformImage can cause a bogus memory allocation size since 0.19.0
Comment 1 Thomas Freitag 2012-05-05 03:36:03 UTC
Created attachment 61073 [details] [review]
Avoid bogus memory allocation size

Here the patch for it. Regtest is started, but propbably I've to leave home before it finishes. I'll give an additional comment on the result of my regtest today evening or tommorow early morning.
Comment 2 Thomas Freitag 2012-05-05 23:06:40 UTC
Regtest passed. There a very few changed output on pages with rotated images, but without any visuable loss of quality.
Comment 3 Albert Astals Cid 2012-05-10 06:31:52 UTC
Do you have 
md5sum bug176320.pdf
ce4a048e12282f22f25f6721c15c2fbf  bug176320.pdf
around?

I see a visible regression in the bride fingers in page 4
Comment 4 Thomas Freitag 2012-05-10 06:52:05 UTC
(In reply to comment #3)
> Do you have 
> md5sum bug176320.pdf
> ce4a048e12282f22f25f6721c15c2fbf  bug176320.pdf
> around?
> 
> I see a visible regression in the bride fingers in page 4
Yes, I have it, and there were a few more changes, i.e. Notebook- Media Kit.pdf on page 12 and 16. But I can't really remember a loss of quality. And I'm on a business trip this week, so I can't look at it once again on before saturday morning.
But as far as I remember, when You look at scaledWidth and scaledHeight: if scaledHeight is really greater than 10000, it was always a calculation error, and scaledHeight was MUCH greater than scaledWidth, and the result was that it was downscaled later dramatically, which results now in a few regressions, but in my opinion without any loss of quality.
Comment 5 Thomas Freitag 2012-05-12 00:34:12 UTC
Created attachment 61476 [details] [review]
Avoid bogus memory allocation size

You're really great in detection of regressions: I must admit that when I had a first look at the outputs today morning I hadn't any clue what You mean. Only when I encountered in the debugger that it is not only one image but that it is striped I saw what You meant.
Then I first tried to remove my changes of xpdf code, but the result wasn't even better. With the new approach now I just use the calculation of th to detect if the image is rotated, and if so, I double the scaled height to ensure that we have enough pixels. This solves the problem why I introduced it (Algorithmics - The Spirit of Computing, 3rd Ed.pdf, page 284 and 288, at least when rendering it with 150 dpi we get an acceptable quality), it still renders the PDF from Ralph and because the factor is now always the same we again get an acceptable quality in bug176320.pdf.
I started my regtest, but I don't want to wait until it finishes because I have to leave (tennis season has started :-) ), perhaps You'll find the time to regtest it, too, You're much better in finding regressions, and we can discuss it tomorrow afternoon.
Comment 6 Thomas Freitag 2012-05-12 22:59:59 UTC
Forget the patch: I found an inacceptable regression on page 11 of bug-poppler10402.pdf. I fear I have to stop guessing but try to understand what is going wrong :-(
Comment 7 Thomas Freitag 2012-05-17 00:16:54 UTC
Created attachment 61739 [details] [review]
Avoid bogus memory allocation size

The attached patch now runs successfully through my regression test. But be aware, in round about 90 samples it changes the output. The reason for it is that I replaced the algorithm for calculating scaledWidth and scaledHeight (before rotating) with a better one and use the old one only if the calculated values are unusable or we run into the special case of tiling patterns.
I could do that also the other way round, use the old algorithm taking the new one only if we run into memory problems. But this would cause
a) that we use sometimes much more pixel than needed and so slow down rendering and
b) that in case of striped images where the stripes varies around the memory limit we will get a worse output because using different calculations.

I looked in every changed output, and where I thought that I got perhaps a worse output I rendered it once again with 150 dpi instead of 72 dpi and finally accept the changed output.
Comment 8 Thomas Freitag 2012-05-31 00:34:34 UTC
*** Bug 50514 has been marked as a duplicate of this bug. ***
Comment 9 Albert Astals Cid 2012-06-10 08:58:37 UTC
There's a file to reproduce in bug 50514
Comment 10 Ralph 2012-06-12 11:50:36 UTC
*** Bug 51014 has been marked as a duplicate of this bug. ***
Comment 11 Thomas Freitag 2012-06-25 00:50:37 UTC
Created attachment 63422 [details] [review]
Avoid bogus memory allocation size

Rebased patch after commit f48eb669ae5c729c026554802e666e64399c0900
Comment 12 Albert Astals Cid 2012-07-12 20:54:36 UTC
Being picky, the wheel of the motorbike in page 1 of bug280330.pdf is no longer a circle with this patch
Comment 13 Albert Astals Cid 2012-07-12 22:57:53 UTC
After having a look at the rest of files i could not find anything worse (though i remember having a real objection but can't find it anymore) so i've commited it to both 0.20 and master, if you can make that wheel a bit rounder it'd be cool though

Thanks for the patience!
Comment 14 Fabio D'Urso 2012-07-25 14:54:04 UTC
*** Bug 52487 has been marked as a duplicate of this bug. ***

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.