Bug 213828 – [CG] Implement image transcoding

• Component: Images
• Status: RESOLVED DUPLICATE

[Said Abou-Hallawa]

Implement a function that transcodes an image.

Source: one of the formats that CG supports for decoding.
Destination: one of the formats that CG supports for encoding.

The output will be a temporary file with the preferred extension of the destination type.

[Said Abou-Hallawa]

Created attachment 403280 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403305 [details]
Patch

[Darin Adler]

Comment on attachment 403305 [details]
Patch

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

> Source/WTF/wtf/cocoa/FileSystemCocoa.mm:116
> +    if (pathMinusPrefixLength > PATH_MAX)

I believe there can be limits on filenames that are much more restrictive than PATH_MAX, which is a limit on the length of a full path passed to POSIX API.

[Said Abou-Hallawa]

Comment on attachment 403305 [details]
Patch

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

>> Source/WTF/wtf/cocoa/FileSystemCocoa.mm:116
>> +    if (pathMinusPrefixLength > PATH_MAX)
> 
> I believe there can be limits on filenames that are much more restrictive than PATH_MAX, which is a limit on the length of a full path passed to POSIX API.

What are these limits? Can you please explain? I thought I was fixing a bug in the existing code based on your review in https://bugs.webkit.org/show_bug.cgi?id=213347#c23.

[Darin Adler]

For example, the historical "HFS+" file system on macOS had a limit of 255 UTF-16 characters for filenames. That can’t be turned into a specific limit on UTF-8, because it depends on which character. PATH_MAX is way higher than 255. Older Mac file systems had even shorter limits. If the data is in a directory not guaranteed to be on the boot volume, there could be even more variation in what kind of file system it’s on and what the limits are.

Here’s a page talking a little bit about this just on Mac.

https://apple.stackexchange.com/questions/86611/does-os-x-enforce-a-maximum-filename-length-or-character-restriction

Seems possible this changed with APFS. Also, not sure where this directory is. Probably less risk of different types of file systems if the file is on the boot volume, but also if the name comes from a different file system, then they name might *already* be too long to just reuse it on the boot volume.

Generally we can’t take an existing file name and make it longer and expect it to still work. There’s no trivial "this is the length maximum" trick that I know of either. Safest is to find a way to never invent a filename based on another filename. If we really have to translate one into another we can probably find some technique that prevents us from hitting limits, but we probably need to test at the edge cases. Maybe construct the longest possible UTF-16 filename, and also the longest possible UTF-8 filename. Unfortunately even making those tests work portably can be a pain.

I would typically sidestep this by trying to make sure that the local filename for any temporary file is randomly generated and does not have any effect on what the user sees. But maybe that’s not practical either!

Maybe we can just stick with this "make the filename longer" thing because it won’t be a headache in practice, but then we have to still test the edge case to see that the error handling code doesn’t do anything crazy. Ideally it makes it clear that the length of the name is the problem.

[Darin Adler]

Comment on attachment 403305 [details]
Patch

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

>>> Source/WTF/wtf/cocoa/FileSystemCocoa.mm:116
>>> +    if (pathMinusPrefixLength > PATH_MAX)
>> 
>> I believe there can be limits on filenames that are much more restrictive than PATH_MAX, which is a limit on the length of a full path passed to POSIX API.
> 
> What are these limits? Can you please explain? I thought I was fixing a bug in the existing code based on your review in https://bugs.webkit.org/show_bug.cgi?id=213347#c23.

Added some more comments in the bug.

[Said Abou-Hallawa]

Created attachment 403545 [details]
Patch 212489 213828

[Said Abou-Hallawa]

The reasons for trying to create the temporary file with a prefix derived from the original file with the extension of the transcoded image were to be able to recognize it in Finder while debugging and be able to preview it. But there is no actual technical reason to stick with this plan since CG is capable of determining the right format of the image even if the file has no file extension or if it has the wrong file extension.

So I changed the patch to create the temporary file the same way other places are doing. A literal prefix is passed without any file extension to FileSystem::openTemporaryFile().

[Said Abou-Hallawa]

Created attachment 403546 [details]
Patch for review

[Said Abou-Hallawa]

No code is using the new function. So I combined transcoding function with the patch of bug 213347. So I am resolving the bug as a dupe of the other bug.

*** This bug has been marked as a duplicate of bug 213347 ***