Bug 178502 – [CG] PostScript images should be supported if they are sub-resource images

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

1. Open the attached EPS image green-100x100.esp

Result: A 100x100 green rectangle is displayed.

2. Open the attached HTML page green-100x100.html

Result: A broken image is displayed.

The PostScript images are detected and converted to images PDF when they are the main resources. When they are sub-resources, WebKit tries to decode them as if they were binary images.

[Said Abou-Hallawa]

Created attachment 324198 [details]
green-100x100.eps

[Said Abou-Hallawa]

Created attachment 324199 [details]
green-100x100.html

[Said Abou-Hallawa]

Created attachment 324201 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/35000589>

[Sam Weinig]

Comment on attachment 324201 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Let PDFDocumentImage replaces the PostScript data with the equivalent PDF
> +        data and use it when creating the PDFDocument.

Why are we doing this conversion? Is it because ImageIO doesn't support PostScript directly?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:374
> +    CGPSConverterCallbacks callbacks = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +    RetainPtr<CGPSConverterRef> converter = adoptCF(CGPSConverterCreate(0, &callbacks, 0));
> +    RetainPtr<CGDataProviderRef> provider = adoptCF(CGDataProviderCreateWithCFData(postScriptData.get()));
> +    RetainPtr<CFMutableDataRef> pdfData = adoptCF(CFDataCreateMutable(kCFAllocatorDefault, 0));
> +    RetainPtr<CGDataConsumerRef> consumer = adoptCF(CGDataConsumerCreateWithCFData(pdfData.get()));

Thes should use auto.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:376
> +    CGPSConverterConvert(converter.get(), provider.get(), consumer.get(), 0);

Is this operation expensive?  Should we be doing it on a background thread, asynchronously?  What happens with a really big PostScript file?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:51
> +    static Ref<PDFDocumentImage> create(ImageObserver* observer, bool isPostScript = false)

I think this would be clearer as an enum:

enum class Type {
    PDF,
    PostScript
};

[Tim Horton]

Comment on attachment 324201 [details]
Patch

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

> LayoutTests/fast/images/resources/green-100x100.eps:1
> +%!PS-Adobe-3.0 EPSF-3.0

This is an insanely overcomplicated EPS, I emailed you a better (<200byte) one.

[Said Abou-Hallawa]

Created attachment 324276 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 14 files


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

[Said Abou-Hallawa]

Comment on attachment 324201 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        data and use it when creating the PDFDocument.
> 
> Why are we doing this conversion? Is it because ImageIO doesn't support PostScript directly?

Yes ImageIO handles only the bitmap images. SVG is handled completely by WebKit. PDF is handled by the PDFKit. So the easiest way to have PostScript handled was to convert it to PDF and let PDFKit render it.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:374
>> +    RetainPtr<CGDataConsumerRef> consumer = adoptCF(CGDataConsumerCreateWithCFData(pdfData.get()));
> 
> Thes should use auto.

Done.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:376
>> +    CGPSConverterConvert(converter.get(), provider.get(), consumer.get(), 0);
> 
> Is this operation expensive?  Should we be doing it on a background thread, asynchronously?  What happens with a really big PostScript file?

Yes I think so. At least it will as slow as decoding the data uri images which is decoded asynchronously. So I will log a separate bug to convert the sub-resource PostScript to a PDF asynchronously.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:51
>> +    static Ref<PDFDocumentImage> create(ImageObserver* observer, bool isPostScript = false)
> 
> I think this would be clearer as an enum:
> 
> enum class Type {
>     PDF,
>     PostScript
> };

Done.

>> LayoutTests/fast/images/resources/green-100x100.eps:1
>> +%!PS-Adobe-3.0 EPSF-3.0
> 
> This is an insanely overcomplicated EPS, I emailed you a better (<200byte) one.

Thanks. This was very helpful. But I had to tweak it a little to get the green rectangle without anti-aliasing at the borders.

[Said Abou-Hallawa]

Created attachment 324325 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 15 files


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

[Build Bot]

Comment on attachment 324325 [details]
Patch

Attachment 324325 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4926892

New failing tests:
fast/images/eps-support-sub-resource.html

[Build Bot]

Created attachment 324335 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Said Abou-Hallawa]

Created attachment 324431 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 17 files


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

[Build Bot]

Comment on attachment 324431 [details]
Patch

Attachment 324431 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4936524

New failing tests:
webrtc/video-replace-muted-track.html

[Build Bot]

Created attachment 324447 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Said Abou-Hallawa]

<rdar://problem/35102988>

[Said Abou-Hallawa]

Created attachment 324449 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 17 files


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

[Tim Horton]

Comment on attachment 324449 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:92
> +#if ENABLE(PDFKIT_PLUGIN)

Why is this in ENABLE(PDFKIT_PLUGIN)?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:366
> +#if ENABLE(PDFKIT_PLUGIN)

Why is this in ENABLE(PDFKIT_PLUGIN)?

[Said Abou-Hallawa]

Created attachment 324476 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 17 files


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

[Said Abou-Hallawa]

Comment on attachment 324449 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:92
>> +#if ENABLE(PDFKIT_PLUGIN)
> 
> Why is this in ENABLE(PDFKIT_PLUGIN)?

I replaced it by #if PLATFORM(MAC). And I made sure that we do not create a PDFDocumentImage unless the data is for a PDF image or the data is for an EPS image and the platform is Mac.

[Build Bot]

Comment on attachment 324476 [details]
Patch

Attachment 324476 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4938954

New failing tests:
webrtc/video-replace-muted-track.html

[Build Bot]

Created attachment 324479 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Tim Horton]

(In reply to Said Abou-Hallawa from comment #24)
> Comment on attachment 324449 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324449&action=review
> 
> >> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:92
> >> +#if ENABLE(PDFKIT_PLUGIN)
> > 
> > Why is this in ENABLE(PDFKIT_PLUGIN)?
> 
> I replaced it by #if PLATFORM(MAC). And I made sure that we do not create a
> PDFDocumentImage unless the data is for a PDF image or the data is for an
> EPS image and the platform is Mac.

I don't understand why we wouldn't want this on iOS as well.

[Said Abou-Hallawa]

Created attachment 324909 [details]
Patch

[Build Bot]

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


ERROR: Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:52:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 17 files


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

[Said Abou-Hallawa]

(In reply to Tim Horton from comment #27)
> (In reply to Said Abou-Hallawa from comment #24)
> > Comment on attachment 324449 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=324449&action=review
> > 
> > >> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:92
> > >> +#if ENABLE(PDFKIT_PLUGIN)
> > > 
> > > Why is this in ENABLE(PDFKIT_PLUGIN)?
> > 
> > I replaced it by #if PLATFORM(MAC). And I made sure that we do not create a
> > PDFDocumentImage unless the data is for a PDF image or the data is for an
> > EPS image and the platform is Mac.
> 
> I don't understand why we wouldn't want this on iOS as well.

Because the header file CGPSConverter.h exists only in the Mac CoreGraphics framework. This header file contains all the definitions of the structures and APIs we use in PDFDocumentImage::convertPostScriptDataToPDF().

[Said Abou-Hallawa]

Created attachment 325182 [details]
Patch

[Said Abou-Hallawa]

Created attachment 325183 [details]
Patch

[Said Abou-Hallawa]

Created attachment 325185 [details]
Patch

[Said Abou-Hallawa]

Created attachment 325506 [details]
Patch

[Tim Horton]

Comment on attachment 325506 [details]
Patch

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

> Source/WebCore/loader/cache/CachedImage.cpp:315
> +        return url().path().endsWith(".pdf", false);

This random extension checking seems weird, do we have any prior examples?

[Said Abou-Hallawa]

Created attachment 327768 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 325506 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedImage.cpp:315
>> +        return url().path().endsWith(".pdf", false);
> 
> This random extension checking seems weird, do we have any prior examples?

In WebPage::createPlugin() we have the following if-statment:

if (shouldUsePDFPlugin() && (MIMETypeRegistry::isPDFOrPostScriptMIMEType(parameters.mimeType) || (parameters.mimeType.isEmpty() && (path.endsWithIgnoringASCIICase(".pdf") || path.endsWithIgnoringASCIICase(".ps")))))

[Simon Fraser (smfr)]

Comment on attachment 327768 [details]
Patch

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

> Source/WebCore/loader/cache/CachedImage.h:98
> +    bool isPDFRequest() const;
> +    bool isPostScriptRequest() const;

It's odd to ask whether an image is a "request". Maybe isPDFResource()/isPostScriptResource()?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:363
> +    CGPSConverterCallbacks callbacks = { 0, 0, 0, 0, 0, 0, 0, 0 };

Could be CGPSConverterCallbacks callbacks = { };

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:369
> +    CGPSConverterConvert(converter.get(), provider.get(), consumer.get(), 0);

How expensive is this? Can it happen off the main thread?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:58
> +    WEBCORE_EXPORT static RetainPtr<CFMutableDataRef> convertPostScriptDataToPDF(RetainPtr<CFDataRef> postScriptData);

This should return a CFDataRef, right? The caller doesn't need mutability.

Can the argument be a  RetainPtr<CFDataRef>&& ?

[Said Abou-Hallawa]

Created attachment 327778 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 327768 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedImage.h:98
>> +    bool isPostScriptRequest() const;
> 
> It's odd to ask whether an image is a "request". Maybe isPDFResource()/isPostScriptResource()?

Functions were renamed.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:363
>> +    CGPSConverterCallbacks callbacks = { 0, 0, 0, 0, 0, 0, 0, 0 };
> 
> Could be CGPSConverterCallbacks callbacks = { };

Done.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:369
>> +    CGPSConverterConvert(converter.get(), provider.get(), consumer.get(), 0);
> 
> How expensive is this? Can it happen off the main thread?

It is expensive operation for sure. I logged https://bugs.webkit.org/show_bug.cgi?id=180104 to track this issue.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:58
>> +    WEBCORE_EXPORT static RetainPtr<CFMutableDataRef> convertPostScriptDataToPDF(RetainPtr<CFDataRef> postScriptData);
> 
> This should return a CFDataRef, right? The caller doesn't need mutability.
> 
> Can the argument be a  RetainPtr<CFDataRef>&& ?

Yes the argument can be RetainPtr<CFDataRef>&&. But I could not change the return value because we assign the value of convertPostScriptDataToPDF() to PDFPlugin::m_data which is of type RetainPtr<CFMutableDataRef>. I can't change its type because PDFPlugin CFDataAppendBytes to the m_data at some point.

[WebKit Commit Bot]

Comment on attachment 327778 [details]
Patch

Clearing flags on attachment: 327778

Committed r225241: <https://trac.webkit.org/changeset/225241>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.