Bug 54406 – png-1.5 fixes

• Component: Images
• Status: RESOLVED FIXED

[Thomas Klausner]

Created attachment 82349 [details]
Patch fixing the problem, needs png-1.5.1.

png-1.5 hides structure members from public view. For this reason, some code doesn't compile any more.

The attached patch, mostly from John Bowler <[e-mail]> fixes the problem.
Some discussion on the solution:
https://sourceforge.net/mailarchive/forum.php?thread_name=000a01cbb6d5%241c18d5b0%24544a8110%24%40acm.org&forum_name=png-mng-implement

Since webkit-gtk used internal data that wasn't available via the public interface, the interface has been extended. The patch already makes use of it, which requires png-1.5.1 though. I'm not sure how you want to handle that.

[Thomas Klausner]

One more thing -- the patch is against webkit-gtk-1.2.7.

[Patrick R. Gansterer]

View in context: https://bugs.webkit.org/attachment.cgi?id=82349&action=review

Welcome to the WebKit bugzilla and thx for the patch.
Every patch requries a ChangeLog and should be based on the trunk version, so it can apply on the tree. For more information about contributing to WebKit see http://www.webkit.org/coding/contributing.html. FYI: If you set the review flag to "?" your patch gets better attention and the EWS (the bubbles beside the attachment) fill process it.

> WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:67
> +#if (PNG_LIBPNG_VER < 10500)
>      longjmp(JMPBUF(png), 1);
> +#else
> +    png_longjmp(png, 1);
> +#endif

You use this many times in you code. IMHO sth like the following on top of the file would be better:
#if (PNG_LIBPNG_VER >= 10500)
static void png_longjmp(png_structp png, int value)
{
     longjmp(JMPBUF(png), value)
}
#endif

> WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:292
> -        m_reader->setReadOffset(m_reader->currentBufferSize() - png->buffer_size);
> -        png->buffer_size = 0;
> +        m_reader->setReadOffset(m_reader->currentBufferSize() - png_process_data_pause(png, 0/*do not save the data*/));

png_process_data_pause doesn't exist in the "old" libpng.

> WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:320
> -        if (m_reader->pngPtr()->interlaced)
> +        if (png_get_interlace_type(m_reader->pngPtr(), m_reader->infoPtr())
> +		!= PNG_INTERLACE_NONE)
>              m_reader->createInterlaceBuffer((m_reader->hasAlpha() ? 4 : 3) * size().width() * size().height());

Please write the if in only one line.

[Thomas Klausner]

Created attachment 83967 [details]
Patch implementing review comments

Thanks for your comments, Patrick. I've addressed all the points you raised, but I couldn't test it since my glib2 isn't new enough.
Please review.

[WebKit Review Bot]

Attachment 83967 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:51:  png_longjmp is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Thomas Klausner]

Re comment #4: The function is called png_longjmp in png-1.5, and the patch adds a replacement for it when compiling against older png versions, per Patrick's suggestion.
Should I really handle this differently?

[WebKit Review Bot]

Attachment 83967 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8024767

[Thomas Klausner]

Created attachment 83968 [details]
Patch addressing review problems.

Addresses compilation problem and style issue.

[WebKit Review Bot]

Attachment 83968 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8070043

[Collabora GTK+ EWS bot]

Attachment 83968 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8031984

[Thomas Klausner]

Created attachment 83969 [details]
Patch fixing bug in previous.

[Patrick R. Gansterer]

Comment on attachment 83969 [details]
Patch fixing bug in previous.

View in context: https://bugs.webkit.org/attachment.cgi?id=83969&action=review

Why does http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp#L134 compile?
What's the exact compile error? I'm not sure it the longjmp part is really required.

> Source/WebCore/ChangeLog:6
> +        Fix compilation with png-1.5, and use new API to terminate data
> +        processing.

Please include the bug number.
If you have a look at the other changelogs the first line is usually the bug title, followed by the bug# and then a detailed description.
Please also include more information about what's the new api and why it doesn't compile any more (some mebers moved from public to private).
You can pass the bug# via "--bug" to preparechangelog.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:50
> +static void pnglongjmp(png_structp png, int value)

Please don't use png as prefix. This function isn't part of libpng. Maybe you can use the first approach with png_longjmp and ignore the style warning, or use a function name without a png prefix. Adding a inline keyword would be good too.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:56
> +#if defined(PNG_LIBPNG_VER_MAJOR) && defined(PNG_LIBPNG_VER_MINOR) && (PNG_LIBPNG_VER_MAJOR > 1 || (PNG_LIBPNG_VER_MAJOR == 1 && PNG_LIBPNG_VER_MINOR >= 5))
> +    png_longjmp(png, value);
> +#else
> +    longjmp(JMPBUF(png), value);
> +#endif

Can we merge this with the #define of JMPBUF ?

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:332
> +        m_reader->setReadOffset(m_reader->currentBufferSize() - png_process_data_pause(png, 0/*do not save the data*/));

Please don't use this "inline" comment. If you think the comment is required, write it in the line above as a one line comment (//) as a full sentence.

[Thomas Klausner]

Created attachment 83972 [details]
Patch implementing review comments

Thanks for the feedback, Patrick.

I've taken out the longjmp changes; they are not necessary. I was following the new png style guide for this kind of code.

I've also implemented all your other suggestions. New patch attached.

(What does the color purple in the bubbles mean, like for "mac" for the previous patch?)

[Patrick R. Gansterer]

Comment on attachment 83972 [details]
Patch implementing review comments

(In reply to comment #12)
> Thanks for the feedback, Patrick.
Thanks for the patch!

> What does the color purple in the bubbles mean, like for "mac" for the previous patch?
You don't have commit rights for the SVN repository (no committer) and the mac bots cannot process patches from non-committers. You can click on them for more info. That's ok!

Also setting cq?. So a reviewer will (usually) set r+ _and_ cq+ on the patch and the commit-queue will commit it without further work.

[David Levin]

*** Bug 51041 has been marked as a duplicate of this bug. ***

[David Levin]

Comment on attachment 83972 [details]
Patch implementing review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=83972&action=review

Just a minor fix up needed and then this can go in.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:323
> +        /* '0' argument to png_process_data_pause means: do not save the data */

Use C++ style comments and whole sentences.

// '0' argument to png_process_data_pause means: Do not save the data.

I was confused by what "saving the data" referred to. From reading http://libpng.git.sourceforge.net/git/gitweb.cgi?p=libpng/libpng;a=blobdiff;f=libpng-manual.txt;h=3caff8f9b7019891fd3d0b08ce134ea7e99140e2;hp=b34ffcc1343407687d6723f1132e69f0cdc34941;hb=0a5c9c02faf9c6ceb7acc40e6219e04194581287;hpb=155ce4021812da5629aaebcc9484094944e7d7ae, it means that png should not cache unprocessed bytes.


Perhaps something like this instead?
  m_reader->setReadOffset(m_reader->currentBufferSize() - png_process_data_pause(png, 0 /* Don't cache unprocessed data. */ ));

[David Levin]

*** Bug 51041 has been marked as a duplicate of this bug. ***

[David Levin]

Just adding myself to the cc line, so I'll see any new patch uploads.

[Thomas Klausner]

Created attachment 87118 [details]
Patch implementing review comments by David Levin

David, thanks for your feedback.

I've changed the comment as suggested.
I didn't make it an inline comment again, since Patrick requested previously to avoid them.

[WebKit Commit Bot]

Comment on attachment 87118 [details]
Patch implementing review comments by David Levin

Clearing flags on attachment: 87118

Committed r82198: <http://trac.webkit.org/changeset/82198>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82198 might have broken Chromium Win Release

[David Levin]

This got rolled out.

Due to Chromium Windows build break:

5>..\platform\image-decoders\png\PNGImageDecoder.cpp(244) : error C3861: 'wk_png_get_image_width': identifier not found
4>ppb_pdf_impl.cc
5>..\platform\image-decoders\png\PNGImageDecoder.cpp(245) : error C3861: 'wk_png_get_image_height': identifier not found
4>ppb_font_impl.cc
4>ppb_video_decoder_impl.cc
5>..\platform\image-decoders\png\PNGImageDecoder.cpp(351) : error C3861: 'wk_png_get_interlace_type': identifier not found

which could be seen here: http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/11702/steps/compile-webkit/logs/stdio

[David Levin]

We should reland this after http://codereview.chromium.org/6708113 lands and we change WebKit/chromium/DEPS to point to that revision of late.

[Thomas Klausner]

The referenced chromium patch has landed as
http://src.chromium.org/viewvc/chrome?view=rev&revision=79710
What next?

[David Levin]

Let me roll deps and land this in about 7 hours which should be off peak time for commits.

[David Levin]

Comment on attachment 87118 [details]
Patch implementing review comments by David Levin

Since DEPS have been rolled, thanks to Tony. I'll r+, cq+ this. Thanks for your patience.

[WebKit Commit Bot]

Comment on attachment 87118 [details]
Patch implementing review comments by David Levin

Rejecting attachment 87118 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-a..." exit_code: 1

Last 500 characters of output:
autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 60] Operation timed out>

Full output: http://queues.webkit.org/results/8285495

[WebKit Commit Bot]

Comment on attachment 87118 [details]
Patch implementing review comments by David Levin

Clearing flags on attachment: 87118

Committed r82344: <http://trac.webkit.org/changeset/82344>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nezmer]

FYI, I needed this too to avoid conversion error:

 #ifdef PNG_iCCP_SUPPORTED
     char* profileName;
     int compressionType;
-    char* profile;
+    png_byte* profile;
     png_uint_32 profileLength;
     if (png_get_iCCP(png, info, &profileName, &compressionType, &profile, &profileLength)) {
         ColorProfile colorProfile;

[WebKit Review Bot]

http://trac.webkit.org/changeset/82344 might have broken GTK Linux 32-bit Release
The following tests are not passing:
css1/basic/comments.html
css1/basic/containment.html
css1/basic/id_as_selector.html
css1/basic/inheritance.html
css1/box_properties/border.html
css1/box_properties/border_bottom.html
css1/box_properties/border_bottom_width.html
css1/box_properties/border_left.html
css1/box_properties/border_left_width.html
css1/box_properties/border_right_inline.html
css1/box_properties/border_right_width.html
css1/box_properties/border_style.html
css1/box_properties/border_top.html
css1/box_properties/border_top_width.html
css1/box_properties/border_width.html
css1/box_properties/clear.html
css1/box_properties/clear_float.html
css1/box_properties/float_elements_in_series.html
css1/box_properties/float_margin.html
css1/box_properties/float_on_text_elements.html
css1/box_properties/height.html
css1/box_properties/margin.html
css1/box_properties/margin_bottom.html
css1/box_properties/margin_inline.html
css1/box_properties/margin_left.html
css1/box_properties/margin_right.html
css1/box_properties/margin_top.html
css1/box_properties/padding.html
css1/box_properties/padding_bottom.html
css1/box_properties/padding_inline.html
css1/box_properties/padding_left.html
css1/box_properties/padding_right.html
css1/box_properties/padding_top.html
css1/box_properties/width.html
css1/cascade/cascade_order.html
css1/classification/display.html
css1/classification/list_style_type.html
css1/classification/white_space.html
css1/color_and_background/background.html
css1/color_and_background/background_attachment.html
css1/color_and_background/background_position.html
css1/color_and_background/background_repeat.html
css1/conformance/forward_compatible_parsing.html
css1/font_properties/font.html
css1/font_properties/font_family.html
css1/font_properties/font_size.html
css1/font_properties/font_weight.html
css1/formatting_model/floating_elements.html
css1/formatting_model/height_of_lines.html
css1/formatting_model/horizontal_formatting.html
css1/formatting_model/inline_elements.html
css1/formatting_model/replaced_elements.html
css1/formatting_model/vertical_formatting.html
css1/pseudo/anchor.html
css1/pseudo/firstletter.html
css1/pseudo/firstline.html
css1/pseudo/multiple_pseudo_elements.html
css1/text_properties/letter_spacing.html
css1/text_properties/line_height.html
css1/text_properties/text_decoration.html
css1/text_properties/text_indent.html
css1/text_properties/text_transform.html
css1/text_properties/vertical_align.html
css1/text_properties/word_spacing.html
css1/units/color_units.html
css1/units/length_units.html
css2.1/t0803-c5502-mrgn-r-02-c.html
css2.1/t0803-c5505-mrgn-02-c.html
css2.1/t080301-c411-vt-mrgn-00-b.html
css2.1/t0905-c5525-fltclr-00-c-ag.html
css2.1/t0905-c5525-fltmrgn-00-c-ag.html
css2.1/t0905-c5525-fltwidth-00-c-g.html
css2.1/t0905-c5526-fltclr-00-c-ag.html
css2.1/t1002-c5523-width-02-b-g.html
css2.1/t1202-counters-08-b.html
css2.1/t1202-counters-09-b.html
css2.1/t140201-c535-bg-fixd-00-b-g.html
css2.1/t140201-c537-bgfxps-00-c-ag.html
css2.1/t1508-c527-font-07-b.html
editing/deleting/delete-after-span-ws-001.html
editing/deleting/delete-after-span-ws-002.html
editing/deleting/delete-after-span-ws-003.html
editing/deleting/delete-line-end-ws-001.html
editing/deleting/delete-line-end-ws-002.html
editing/inserting/insert-div-023.html
editing/selection/focus_editable_html.html
editing/selection/select-all-001.html
editing/selection/select-all-002.html
editing/selection/select-all-003.html
editing/selection/select-all-004.html
editing/selection/unrendered-001.html
editing/selection/unrendered-002.html
editing/selection/unrendered-003.html
editing/selection/unrendered-004.html
editing/selection/unrendered-005.html
fast/text/international/complex-character-based-fallback.html
fast/text/international/thai-line-breaks.html
fonts/cursive.html
fonts/default.html
fonts/fantasy.html
fonts/monospace.html
fonts/sans-serif.html
fonts/serif.html
http/tests/misc/link-rel-icon-beforeload.html
media/video-controls-rendering.html
media/video-zoom.html
tables/mozilla/bugs/bug101674.html
tables/mozilla/bugs/bug10269-2.html
tables/mozilla/bugs/bug10296-1.html
tables/mozilla/bugs/bug1055-1.html
tables/mozilla/bugs/bug113235-1.html
tables/mozilla/bugs/bug113235-3.html
tables/mozilla/bugs/bug11944.html
tables/mozilla/bugs/bug120364.html
tables/mozilla/bugs/bug12384.html
tables/mozilla/bugs/bug1302.html
tables/mozilla/bugs/bug131020.html
tables/mozilla/bugs/bug131020_iframe.html
tables/mozilla/bugs/bug137388-2.html
tables/mozilla/bugs/bug194024.html
tables/mozilla/bugs/bug22019.html
tables/mozilla/bugs/bug23151.html
tables/mozilla/bugs/bug2479-1.html
tables/mozilla/bugs/bug2479-3.html
tables/mozilla/bugs/bug2479-4.html
tables/mozilla/bugs/bug27038-2.html
tables/mozilla/bugs/bug29314.html
tables/mozilla/bugs/bug2947.html
tables/mozilla/bugs/bug32205-2.html
tables/mozilla/bugs/bug38916.html
tables/mozilla/bugs/bug3977.html
tables/mozilla/bugs/bug43039.html
tables/mozilla/bugs/bug43854-1.html
tables/mozilla/bugs/bug44505.html
tables/mozilla/bugs/bug46480-1.html
tables/mozilla/bugs/bug46480-2.html
tables/mozilla/bugs/bug50695-1.html
tables/mozilla/bugs/bug56405.html
tables/mozilla/bugs/bug5797.html
tables/mozilla/bugs/bug5835.html
tables/mozilla/bugs/bug625.html
tables/mozilla/bugs/bug650.html
tables/mozilla/bugs/bug67915-1.html
tables/mozilla/bugs/bug7112-1.html
tables/mozilla/bugs/bug7112-2.html
tables/mozilla/bugs/bug73321.html
tables/mozilla/bugs/bug92143.html
tables/mozilla/bugs/bug96334.html
tables/mozilla/bugs/bug96343.html
tables/mozilla/collapsing_borders/bug41262-3.html
tables/mozilla/core/bloomberg.html
tables/mozilla/core/captions.html
tables/mozilla/core/cell_heights.html
tables/mozilla/core/col_span.html
tables/mozilla/core/col_widths_auto_fix.html
tables/mozilla/core/col_widths_fix_fixPer.html
tables/mozilla/core/nested1.html
tables/mozilla/core/one_row.html
tables/mozilla/core/row_span.html
tables/mozilla/marvin/backgr_index.html
tables/mozilla/marvin/backgr_layers-opacity.html
tables/mozilla/marvin/backgr_position-table.html
tables/mozilla/marvin/backgr_simple-table-cell.html
tables/mozilla/marvin/backgr_simple-table-column-group.html
tables/mozilla/marvin/backgr_simple-table-column.html
tables/mozilla/marvin/backgr_simple-table-row-group.html
tables/mozilla/marvin/backgr_simple-table-row.html
tables/mozilla/marvin/backgr_simple-table.html
tables/mozilla/marvin/x_table_bgcolor_name.xml
tables/mozilla/marvin/x_table_bgcolor_rgb.xml
tables/mozilla/marvin/x_td_bgcolor_name.xml
tables/mozilla/marvin/x_td_bgcolor_rgb.xml
tables/mozilla/marvin/x_td_height.xml
tables/mozilla/marvin/x_td_nowrap.xml
tables/mozilla/marvin/x_th_bgcolor_name.xml
tables/mozilla/marvin/x_th_bgcolor_rgb.xml
tables/mozilla/marvin/x_th_height.xml
tables/mozilla/marvin/x_th_nowrap.xml
tables/mozilla/marvin/x_tr_bgcolor_name.xml
tables/mozilla/marvin/x_tr_bgcolor_rgb.xml
tables/mozilla/other/cell_widths.html
tables/mozilla/other/nestedTables.html
tables/mozilla/other/test3.html
tables/mozilla/other/test6.html
tables/mozilla/other/wa_table_thtd_rowspan.html
tables/mozilla/other/wa_table_tr_align.html
tables/mozilla_expected_failures/bugs/bug10140.html
tables/mozilla_expected_failures/bugs/bug101759.html
tables/mozilla_expected_failures/bugs/bug10216.html
tables/mozilla_expected_failures/bugs/bug1055-2.html
tables/mozilla_expected_failures/bugs/bug106966.html
tables/mozilla_expected_failures/bugs/bug131020-3.html
tables/mozilla_expected_failures/bugs/bug14007-1.html
tables/mozilla_expected_failures/bugs/bug14007-2.html
tables/mozilla_expected_failures/bugs/bug19526.html
tables/mozilla_expected_failures/bugs/bug220653.html
tables/mozilla_expected_failures/bugs/bug22122.html
tables/mozilla_expected_failures/bugs/bug2479-5.html
tables/mozilla_expected_failures/bugs/bug32205-1.html
tables/mozilla_expected_failures/bugs/bug67915-2.html
tables/mozilla_expected_failures/bugs/bug7243.html
tables/mozilla_expected_failures/bugs/bug80762-2.html
tables/mozilla_expected_failures/bugs/bug85016.html
tables/mozilla_expected_failures/bugs/bug89315.html
tables/mozilla_expected_failures/bugs/bug91057.html
tables/mozilla_expected_failures/core/backgrounds.html
tables/mozilla_expected_failures/core/captions1.html
tables/mozilla_expected_failures/core/captions2.html
tables/mozilla_expected_failures/core/captions3.html
tables/mozilla_expected_failures/core/col_span2.html
tables/mozilla_expected_failures/core/columns.html
tables/mozilla_expected_failures/core/conflicts.html
tables/mozilla_expected_failures/core/standards1.html
tables/mozilla_expected_failures/marvin/backgr_border-table-cell.html
tables/mozilla_expected_failures/marvin/backgr_border-table-column-group.html
tables/mozilla_expected_failures/marvin/backgr_border-table-column.html
tables/mozilla_expected_failures/marvin/backgr_border-table-quirks.html
tables/mozilla_expected_failures/marvin/backgr_border-table-row-group.html
tables/mozilla_expected_failures/marvin/backgr_border-table-row.html
tables/mozilla_expected_failures/marvin/backgr_border-table.html
tables/mozilla_expected_failures/marvin/backgr_fixed-bg.html
tables/mozilla_expected_failures/marvin/backgr_layers-hide.html
tables/mozilla_expected_failures/marvin/backgr_layers-show.html
tables/mozilla_expected_failures/marvin/backgr_position-table-cell.html
tables/mozilla_expected_failures/marvin/backgr_position-table-column-group.html
tables/mozilla_expected_failures/marvin/backgr_position-table-column.html
tables/mozilla_expected_failures/marvin/backgr_position-table-row-group.html
tables/mozilla_expected_failures/marvin/backgr_position-table-row.html
tables/mozilla_expected_failures/marvin/table_overflow_dirty_reflow_row.html
tables/mozilla_expected_failures/marvin/table_overflow_style_reflow_row_sibling.html
tables/mozilla_expected_failures/marvin/table_overflow_style_reflow_tbody_sibling.html
tables/mozilla_expected_failures/other/empty_cells.html
tables/mozilla_expected_failures/other/test4.html
transforms/2d/hindi-rotated.html
transforms/2d/transform-fixed-container.html

[Thomas Klausner]

@Nezmer: usually this is only a warning.
Unconditionally changing the type as you suggest will just shift the warning to users of png<1.5; you need to change it depending on libpng>=1.5 if you want to get rid of it.