[poppler] Bugfix proposal for Shading Type 6 and 7
Thomas Freitag
Thomas.Freitag at kabelmail.de
Thu Oct 14 03:16:41 PDT 2010
Hi Albert!
I discussed this issue with Christian, and I have the following
suggestion now:
1. You should commit the bugfix part of Christian's patch in 0.15.0, so
that it will be part of 0.15.1. To clearify what I mean with the bugfix
part, I attach the bugfix diff I made against 0.15.0 once again. This
should already run over Your regtest, as far as I understand the several
mails here.
2. I take over the improvement part of the patch, where Christian
implements the gouraud shading in Splash. I made several suggestions to
Christian, i.e. that in the Splash library should be nor references to
the Gfx stuff from the poppler library. I will walk over the code and
redesign it a little bit next weekend. The base should be the same.
3. After You create the 0.15.1, I will make a new diff including my
improvements of bug 30436 and the improvements of Cjhristian and upload
that patch to bug 30436.
Is this okay for You (and for Christian, too), or does anyone have other
suggestions?
Best regards,
Thomas
Am 13.10.2010 20:56, schrieb Christian Feuersaenger:
> Am 13.10.2010 20:34, schrieb Albert Astals Cid:
>> A Divendres, 8 d'octubre de 2010, Christian Feuersaenger va escriure:
>>> Am 07.10.2010 23:17, schrieb Albert Astals Cid:
>>>> A Divendres, 1 d'octubre de 2010, Christian Feuersaenger va escriure:
>>>>> Am 13.08.2010 23:43, schrieb Albert Astals Cid:
>>>>>> A Divendres, 30 de juliol de 2010, Albert Astals Cid va escriure:
>>>>>>> A Dimarts, 27 de juliol de 2010, Christian Feuersaenger va
>>>>>>> escriure:
>>>>>>>> Dear Albert,
>>>>>>>>
>>>>>>>> thank you for your time to perform the regression tests!
>>>>>>>>
>>>>>>>> I have fixed the bug; it was a data type problem.
>>>>>>>>
>>>>>>>> Attached you find the fixed version.
>>>>>>>>
>>>>>>>> The file
>>>>>>>> bugfix_shadingtype4567_incremental.patch
>>>>>>>> is relative to the version you used for the regression tests.
>>>>>>>>
>>>>>>>> The file
>>>>>>>> bugfix_shadingtype4567_poppler0.14.patch
>>>>>>>> is relative to poppler-0.14.0-3-gb2427d0 .
>>>>>>>>
>>>>>>>> Thank you for considering my contributions.
>>>>>>> I've ran the regression test with the Splash outputdev and all
>>>>>>> looks
>>>>>>> ok, will have to run it over the cairo and ps outputdevs before
>>>>>>> committing, though it'll take a while since next week i'm going
>>>>>>> to be
>>>>>>> away on holidays.
>>>>>> Bad news, this patch produces a regression in pdftops when
>>>>>> running over
>>>>>> the attached file, that is, the unpatched version creates a ps file
>>>>>> that is valid (gs will open it) and the patched version creates a ps
>>>>>> file that "crashes" gs.
>>>>>>
>>>>>> Do you think you can have a look?
>>>>>>
>>>>>> Albert
>>>>> Dear Albert,
>>>>>
>>>>> I am having difficulties to reproduce the problem. Here is what I
>>>>> did:
>>>>>
>>>>> 1. try to view bug157704.pdf with the standard gs
>>>>> --> crash (see below)
>>>>> 2. call pdftops (using system's version of libpoppler) and open gs on
>>>>> the result
>>>>> --> works without errors (see below)
>>>>> 3. call pdftops using the patched libpoppler and open gs on the
>>>>> result
>>>>> --> works without errors
>>>>> 4. call pdftops of poppler git version of 0.14 without my patch (I
>>>>> have
>>>>> not yet pulled the new version) and open gs on the result
>>>>> --> works without errors
>>>>>
>>>>> Do you have more detailed information about the crash? I fear I am
>>>>> unable to do anything here...
>>>> Wops, works for me now too, i must have done something weird, sorry
>>>> :-/
>>>>
>>>> I'll start the regression test again.
>>>>
>>>> Albert
>>> Ok, thanks for the notice!
> Dear Albert,
>
> thanks for the thorough testing.
>
>> The regression test was successful, though i have some questions i'd
>> like to
>> get answered before commiting the patch.
>>
>> In Gfx::gouraudFillTriangle you removed the check for
>> contentIsHidden, why?
> Hm. It seems as if the test should be there - I must have forgotten to
> include it. Thank you for pointing it out! Could you add it right
> before the fill statement?
>
>> Why do this if everything else in the line is intengers, what's the
>> point of
>> having a 2. there?
>> - color01.c[i] = (color0->c[i] + color1->c[i]) / 2;
>> + color01.c[i] = (color0->c[i] + color1->c[i]) / 2.;
>>
> You are right, that's certainly a waste of time. I should have
> investigated the compiler warnings; sorry about that. Please feel free
> to remove the double suffixes.
>
> Best regards
>
> Christian
>
>
>>>>> Here are the detailed outputs for your reference:
>>>>>
>>>>> for (1):
>>>>> [ludewich] tmp>>gs bug157704.pdf
>>>>> GPL Ghostscript 8.71 (2010-02-10)
>>>>> Copyright (C) 2010 Artifex Software, Inc. All rights reserved.
>>>>> This software comes with NO WARRANTY: see the file PUBLIC for
>>>>> details.
>>>>> Processing pages 1 through 1.
>>>>> Page 1
>>>>> Error: /unknownerror in --run--
>>>>>
>>>>> Operand stack:
>>>>> --dict:6/15(L)--
>>>>>
>>>>> Execution stack:
>>>>> %interp_exit .runexec2 --nostringval-- --nostringval--
>>>>>
>>>>> --nostringval-- 2 %stopped_push --nostringval--
>>>>> --nostringval-- --nostringval-- false 1 %stopped_push 1878
>>>>> 1 3 %oparray_pop 1877 1 3 %oparray_pop 1861 1 3
>>>>> %oparray_pop --nostringval-- --nostringval-- 2 1 1
>>>>> --nostringval-- %for_pos_int_continue --nostringval--
>>>>> --nostringval-- false 1 %stopped_push --nostringval--
>>>>> --nostringval-- --nostringval-- %array_continue --nostringval--
>>>>> false 1 %stopped_push --nostringval-- %loop_continue
>>>>> --nostringval--
>>>>>
>>>>> Dictionary stack:
>>>>> --dict:1153/1684(ro)(G)-- --dict:1/20(G)--
>>>>> --dict:75/200(L)--
>>>>>
>>>>> --dict:75/200(L)-- --dict:108/127(ro)(G)--
>>>>> --dict:288/300(ro)(G)--
>>>>> --dict:22/25(L)-- --dict:6/8(L)-- --dict:21/40(L)--
>>>>> --dict:3/5(L)-- Current allocation mode is local
>>>>> Last OS error: 11
>>>>> GPL Ghostscript 8.71: Unrecoverable error, exit code 1
>>>>>
>>>>> for (2):
>>>>> [ludewich] tmp>>pdftops -?
>>>>> pdftops version 0.12.4
>>>>> [ludewich] tmp>>time pdftops bug157704.pdf
>>>>>
>>>>> real 0m14.925s
>>>>> user 0m13.900s
>>>>> sys 0m0.140s
>>>>> [ludewich] tmp>>gs bug157704.ps
>>>>> GPL Ghostscript 8.71 (2010-02-10)
>>>>> [ ALL OK ]
>>>>>
>>>>> for (3):
>>>>> [ludewich] poppler>>git describe # (with my patch)
>>>>> poppler-0.14.0-18-g821bc2a
>>>>> [ludewich] tmp>>time ~/code/xpdf/poppler/utils/pdftops bug157704.pdf
>>>>>
>>>>> real 0m2.785s
>>>>> user 0m2.540s
>>>>> sys 0m0.130s
>>>>> [ludewich] tmp>>gs bug157704.ps
>>>>> GPL Ghostscript 8.71 (2010-02-10)
>>>>> Copyright (C) 2010 Artifex Software, Inc. All rights reserved.
>>>>> This software comes with NO WARRANTY: see the file PUBLIC for
>>>>> details.
>>>>> [ ALL OK ]
>>>>>
>>>>>
>>>>> for (4):
>>>>> [ludewich] poppler>>git describe # (without my patch using
>>>>> poppler-0.14
>>>>> ) poppler-0.14.0-3-gb2427d0
>>>>> [ludewich] tmp>>time ~/code/xpdf/poppler/utils/pdftops bug157704.pdf
>>>>>
>>>>> real 0m15.844s
>>>>> user 0m13.820s
>>>>> sys 0m0.140s
>>>>> [ludewich] tmp>>gs bug157704.ps
>>>>> GPL Ghostscript 8.71 (2010-02-10)
>>>>> Copyright (C) 2010 Artifex Software, Inc. All rights reserved.
>>>>> This software comes with NO WARRANTY: see the file PUBLIC for
>>>>> details.
>>>>> [ ALL OK ]
>>>>>
>>>>>>> Albert
>>>>>>>
>>>>>>>> Best regards
>>>>>>>>
>>>>>>>> Christian
>>>>>>>>
>>>>>>>> Am 25.07.2010 16:56, schrieb Albert Astals Cid:
>>>>>>>>> A Dissabte, 3 de juliol de 2010, Christian Feuersaenger va
>>>>>>>>> escriure:
>>>>>>>>>> Hello Albert,
>>>>>>>>> Hi
>>>>>>>>>
>>>>>>>>>> I've managed to fix a bug in the Shading Type 6/7 (Coons&
>>>>>>>>>> cubic
>>>>>>>>>> tensor patches) implementation.
>>>>>>>>>>
>>>>>>>>>> The bugfix is small and stable (in my eyes); the poppler-0.14
>>>>>>>>>> branch doesn't implement support for parameterized patch
>>>>>>>>>> shadings.
>>>>>>>>>> I modified the existing implementation accordingly with
>>>>>>>>>> relatively
>>>>>>>>>> few changes.
>>>>>>>>>>
>>>>>>>>>> Attached you find the patch file and the updated test.pdf to see
>>>>>>>>>> the improvement.
>>>>>>>>>>
>>>>>>>>>> The file type4567patch.... also includes the patch of my
>>>>>>>>>> previous
>>>>>>>>>> mail (they only share the same refinement threshold).
>>>>>>>>>>
>>>>>>>>>> The patch should work relative to poppler-0.14.0-3-gb2427d0 .
>>>>>>>>> This patch causes a regression in the attached pdf (the blue area
>>>>>>>>> disappears)
>>>>>>>>>
>>>>>>>>> Albert
>>>>>>>>>
>>>>>>>>>> Best regards
>>>>>>>>>>
>>>>>>>>>> Christian
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> _______________________________________________
>>>>>>>>>> poppler mailing list
>>>>>>>>>> poppler at lists.freedesktop.org
>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>> _______________________________________________
>>>>>>> poppler mailing list
>>>>>>> poppler at lists.freedesktop.org
>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> poppler mailing list
>>>>>>> poppler at lists.freedesktop.org
>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>> _______________________________________________
>>>>> poppler mailing list
>>>>> poppler at lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>> _______________________________________________
>>>> poppler mailing list
>>>> poppler at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>> _______________________________________________
>>> poppler mailing list
>>> poppler at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>> _______________________________________________
>> poppler mailing list
>> poppler at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/poppler
>
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler
>
> .
>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: gouraud-bugfix.diff
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20101014/f22171c2/attachment-0001.ksh>
More information about the poppler
mailing list