Support preview for DDS images

[mkafrin]

Closes #5337

Description

Uses WebGL2 to render DDS images to a canvas, then extract that canvas image to a dataURL that can be passed into the existing logic to render image previews.

This also necessitates adding the rawContents (buffer) to the Image class so we can use that in the conversion later on. This is technically an optimization since we could decode the base64 contents, but that would introduce an additional (and significant) delay in the conversion that is unnecessary.

The alternative option is to enhance the ImageContainer to be able to render to canvas directly, so it can use canvas for DDS and img for anything else. I played around with this and got pretty far, but kept running into issues so decided to push that off for a potential future enhancement. At the end of the day, this solution produces the same results, just a bit slower due to the toDataURL call to convert the canvas to a dataURL for rendering.

Screenshots

Preview of DDS in deletion diff:

Preview of DDS in addition diff:

Preview of DDS in modification diff (in all four modes):

Release notes

Notes:

[sergiou87]

First of all thanks for your contribution! It looks great overall, but we have some concerns about it:

• We see many dependencies installed to get to render the DDS image in a canvas. Given how (seemingly) "simple" the rendering process is for this, could you try to get rid of all new dependencies (except for parse-dds) and just implement the minimum WebGL stuff needed in the dds-converter.ts file?
• We have seen in the past a number of security issues related to GPU work, so we were hoping this WebGL render code could be moved to a utility process that would run independently from the main and renderer processes.

[mkafrin]

Thanks for the review! Both of those suggestions seem like good ideas, I can definitely do them.

[mkafrin]

I pushed commits to remove the gl-shader and a-big-triangle packages, but I've hit a snag with the second request. Correct me if I'm wrong, but from what I'm reading, it seems that the utilityProcess api spawns Node processes. If I'm correct in that assumption, the WebGL rendering can't be offloaded using that, because WebGL is a browser api, and does not exist in Node. Can you think of an alternative path here? And what security concerns do you have regarding it?

[sergiou87]

Oops! You're right, I think in this case we'd need a new BrowserWindow instance with its own process and WebGL rendering context to perform the operation 🤔

Sorry about that 😅 and thanks for cleaning up those two dependencies! ❤️

And what security concerns do you have regarding it?

Nothing specific, just being extra cautious 😅

[mkafrin]

Hmm, I've been working the new BrowserWindow angle, and I think this might be a mistake to pursue. This is a lot of additional complexity and code for not even a specific problem we're trying to solve. There's an additional process (I modeled it similar to the crash process), additional logic in the main process to spawn the new process and handle ipc, additional build step (webpacking the new process code), many new IPCs (one each for ready and quit, one for request to convert DDS, one for response of dataURL), and additional complexity in ImageContainer.

Without any specific security concerns we're solving for, is all that worth it? It certainly makes the code harder to process, follow, and work on. And it adds additional opportunities for failures/exceptions. Plus, all that additional complexity has it's own security risks. Not to mention that these are official and long-standing WebGL APIs which are sandboxed and field-tested with thousands (if not a lot more) of websites. Heck, I'd venture to guess that those APIs have way more eyes on them and testing (including pen-testing) then Electron's process separation, IPC, etc.

I've pushed the changes to a separate branch if you'd like to review the commit and tell me if I'm making this way more complicated than needed or missed something obvious mkafrin@d4c5c16. However, if that's basically on the mark and what would need to be done to support process separation, I think it's worth reviewing whether it's worth it and if there's a real concern we're solving for.

[sergiou87]

Thank you for that! 🙏 I'll discuss your comment with the team and let you know as soon as possible.

[mkafrin]

Thank you for that! 🙏 I'll discuss your comment with the team and let you know as soon as possible.

Any update on this by chance?

[tidy-dev]

@mkafrin We appreciate your patience on this, unfortunately, we have not been able to discuss this yet. It may be some time before we can circle back.

[mkafrin]

No problem! Just wanted to keep up with it. Thanks for the update!

[mkafrin]

Rebased to fix some conflicts and also to remove the type packages in the original commits, rather than having an additional commit on top.

[mkafrin]

Any updates by chance? @tidy-dev