search

adobe-photoshop / generator-assets

Generator Plug-in for Adobe Photoshop CC that helps users export image assets
MIT License
496 stars 86 forks source link

PS Generator Stress Test file for SVG crashes generator-assets #267

Open nimbupani opened 10 years ago

nimbupani commented 10 years ago

The test file is in the trello for Crema.

@rioter says:

My first pass at a fix would be to remove the constant copying of the buffer and instead stream the result around. It is a very risky fix this close to a release.

@jhatwich says:

After chatting it sounds like the fix would go in generator-core and/or generator-assets and essentially it is to rewrite the pipeline to be stream-based anywhere its using buffers. I think this might be a big change due to how "convert" works - it is a bolted-on binary... I think we have to give it the whole image... anyway, this is probably solvable, but may take several weeks of engineering time.

joelrbrandt commented 10 years ago

This is a major change. But yes, it's the correct fix.

I started the work a long time ago in this branch: https://github.com/adobe-photoshop/generator-core/tree/jrb/streaming

The reason it wasn't implemented this way initially is because node v0.8 didn't have streaming implementations of the crypto functions (which are necessary for socket-based communication with Photoshop). Node v0.10 does, but we haven't had the time to rewrite it.

Implementing it to ensure that buffer copies don't happen is challenging, because a.) there are different code paths for sockets and pipes (pipes aren't encrypted) and b.) in the socket case, not all bytes are encrypted (so the entire stream can't just be piped through the crypto stream). Making matters worse, reading a specified number of bytes from a node stream actually results in a buffer copy (of the unread portion, which can be as large as 64MB on mac -- PS dumps pixels quickly, and OS socket buffers are large). So, implementing this correctly involves writing a complicated transform stream that shuttles off the correct number of bytes to all of the correct places.

Anyway, if this is high priority, I might be able to finish my implementation this week. QEing it for shipping in 15.2 is another story, though.

nimbupani commented 10 years ago

It is high priority but it is too much of a risk right now for us to work with this change for MAX. We would like to pursue this immediately after zero-bug day for MAX. How does that work?

joelrbrandt commented 10 years ago

@nimbupani Works fine for me. I'll start (resume) implementing it now so that we have as much of a runway as possible post MAX.

Incidentally, we should lock down version numbers and start ps15.2-release branches in both repos soon, and start integrating from those into perforce (rather than from master). I'll do this soon, too. CC @iwehrman and @timothynoel regarding this fact. (I'll send an email when it's done.)

jaredwy commented 10 years ago

@joelrbrandt i have a pretty good understanding of what needs to happen there, if you need any engineering effort let me know.

mcilroyc commented 8 years ago

Leaving this bug open to make sure it is actually resolved if/when we implement the suggested streaming. I have opened https://jira.corp.adobe.com/browse/PS-1327 to track that effort.