Bug 213828 - [CG] Implement image transcoding
Summary: [CG] Implement image transcoding
Status: RESOLVED DUPLICATE of bug 213347
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords:
Depends on:
Blocks: 213347
  Show dependency treegraph
 
Reported: 2020-06-30 22:20 PDT by Said Abou-Hallawa
Modified: 2022-02-10 16:49 PST (History)
7 users (show)

See Also:


Attachments
Patch (10.85 KB, patch)
2020-07-01 02:29 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (11.58 KB, patch)
2020-07-01 11:28 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch 212489 213828 (30.49 KB, patch)
2020-07-04 20:22 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch for review (5.27 KB, patch)
2020-07-04 21:40 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2020-06-30 22:20:53 PDT
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.
Comment 1 Said Abou-Hallawa 2020-07-01 02:29:53 PDT
Created attachment 403280 [details]
Patch
Comment 2 Said Abou-Hallawa 2020-07-01 11:28:12 PDT
Created attachment 403305 [details]
Patch
Comment 3 Darin Adler 2020-07-01 11:53:34 PDT
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.
Comment 4 Said Abou-Hallawa 2020-07-01 12:04:06 PDT
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.
Comment 5 Darin Adler 2020-07-01 13:06:50 PDT
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.
Comment 6 Darin Adler 2020-07-01 13:38:34 PDT
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.
Comment 7 Said Abou-Hallawa 2020-07-04 20:22:24 PDT
Created attachment 403545 [details]
Patch 212489 213828
Comment 8 Said Abou-Hallawa 2020-07-04 20:31:05 PDT
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().
Comment 9 Said Abou-Hallawa 2020-07-04 21:40:53 PDT
Created attachment 403546 [details]
Patch for review
Comment 10 Said Abou-Hallawa 2020-07-06 13:21:05 PDT
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 ***