problem with chromium-windows and png-1.5 fixes for webkit

[Thomas Klausner]

Hi!

https://bugs.webkit.org/show_bug.cgi?id=54406 was backed out because
it caused https://bugs.webkit.org/show_bug.cgi?id=57304

The compilation problem was:
5>..\platform\image-decoders\png\PNGImageDecoder.cpp(244) : error
C3861: 'wk_png_get_image_width': identifier not found
5>..\platform\image-decoders\png\PNGImageDecoder.cpp(245) : error
C3861: 'wk_png_get_image_height': identifier not found
5>..\platform\image-decoders\png\PNGImageDecoder.cpp(351) : error
C3861: 'wk_png_get_interlace_type': identifier not found

1. The patch in 54406 isn't using wk_png* but png*, where does the
"wk_" come from?

2. Webkit doesn't seem to do any symbol mangling. (Why) Is Chromium
doing that?

I checked two of the functions, png_get_image_height and
png_get_image_width -- they were added to png.h in 1998, so I can only
imagine it must be some symbol mangling problem.

Please advise.

Thanks,
Thomas

[Mark Mentovai]

Thomas Klausner wrote:
> https://bugs.webkit.org/show_bug.cgi?id=54406 was backed out because
> it caused https://bugs.webkit.org/show_bug.cgi?id=57304
>
> The compilation problem was:
> 5>..\platform\image-decoders\png\PNGImageDecoder.cpp(244) : error
> C3861: 'wk_png_get_image_width': identifier not found
> 5>..\platform\image-decoders\png\PNGImageDecoder.cpp(245) : error
> C3861: 'wk_png_get_image_height': identifier not found
> 5>..\platform\image-decoders\png\PNGImageDecoder.cpp(351) : error
> C3861: 'wk_png_get_interlace_type': identifier not found
>
> 1. The patch in 54406 isn't using wk_png* but png*, where does the
> "wk_" come from?

third_party/libpng/pngusr.h. The include chain from PNGImageDecoder.h
is "png.h" and "pngconf.h", both in third_party/libpng.

> 2. Webkit doesn't seem to do any symbol mangling. (Why) Is Chromium
> doing that?

From that file, pngusr.h:

/* Mangle names of exported libpng functions so different libpng versions
can coexist. It is recommended that if you do this, you give your
library a different name such as "libwkpng" instead of "libpng". */

And from README.chromium in that directory:

Our custom configuration options are defined in pngusr.h. This was previously
called mozpngconf.h, which was copied from Mozilla and modified by Apple (hence
the webkit_* names).

The mangling is working correctly and isn’t a problem worth chasing
further. Your compilation failure is caused by Chromium’s pngusr.h
defining PNG_NO_EASY_ACCESS, which removes [wk_]png_get_image_width,
[wk_]png_get_image_height, [wk_]png_get_interlace_type, and others.
Look in third_party/libpng/pngconf.h

[Thomas Klausner]

On Tue, Mar 29, 2011 at 10:55:28AM -0400, Mark Mentovai wrote:
> The mangling is working correctly and isn’t a problem worth chasing
> further. Your compilation failure is caused by Chromium’s pngusr.h
> defining PNG_NO_EASY_ACCESS, which removes [wk_]png_get_image_width,
> [wk_]png_get_image_height, [wk_]png_get_interlace_type, and others.
> Look in third_party/libpng/pngconf.h

Ok, I see.

Could you please change pngusr.h to define PNG_EASY_ACCESS_SUPPORTED
instead of PNG_NO_EASY_ACCESS?

The current webkit sources (without the patch I cited applied) just
work around the define by using the corresponding members of the png
struct directly. When you change the defines, we are at least honest
about it, and can use the functions provided by libpng (and compile
against libpng-1.5).

Thanks,
Thomas

[Mark Mentovai]

Thomas Klausner wrote:
> Could you please change pngusr.h to define PNG_EASY_ACCESS_SUPPORTED
> instead of PNG_NO_EASY_ACCESS?
>
> The current webkit sources (without the patch I cited applied) just
> work around the define by using the corresponding members of the png
> struct directly.  When you change the defines, we are at least honest
> about it, and can use the functions provided by libpng (and compile
> against libpng-1.5).

http://codereview.chromium.org/6708113

[Mark Mentovai]

I checked this in at Chromium r79710.

[Tony Chang]

I pulled this change into webkit in r82310.  Thomas, you should be able to land your patch in WebKit now.

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
   http://groups.google.com/a/chromium.org/group/chromium-dev