Summary: | Splash::arbitraryTransformImage causes bogus memory allocation size | ||
---|---|---|---|
Product: | poppler | Reporter: | Thomas Freitag <Thomas.Freitag> |
Component: | splash backend | Assignee: | 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
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. Regtest passed. There a very few changed output on pages with rotated images, but without any visuable loss of quality. Do you have md5sum bug176320.pdf ce4a048e12282f22f25f6721c15c2fbf bug176320.pdf around? I see a visible regression in the bride fingers in page 4 (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. 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. 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 :-( 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. *** Bug 50514 has been marked as a duplicate of this bug. *** *** Bug 51014 has been marked as a duplicate of this bug. *** Created attachment 63422 [details] [review] Avoid bogus memory allocation size Rebased patch after commit f48eb669ae5c729c026554802e666e64399c0900 Being picky, the wheel of the motorbike in page 1 of bug280330.pdf is no longer a circle with this patch 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! *** 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.