Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faster PNG striping #8

Merged
merged 1 commit into from Sep 13, 2016
Merged

Faster PNG striping #8

merged 1 commit into from Sep 13, 2016

Conversation

rafaelfranca
Copy link
Contributor

I found that when the image is really big the ChunkyPNG gem is slow to encode/decode the PNG pixels. As we only want to strip the metadata we can work in the Datastream that doesn't transform the pixels.

With my test image this method went from 10s to 0.01s.

@@ -52,8 +52,6 @@
png_metadata = ChunkyPNG::Image.from_file(png_path.to_s).metadata
expect(png_metadata).to be_empty

# Metadata usually contains some timestamps, so sleeping is the best way to make sure they are all stripped
sleep 1
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the metadata are properly stripped, how come this break the build?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't know. Theoretical, even before it was being striped correctly
but I think there is something in the data itself that is time dependent
and is not included in the metadata.
On ter, 13 de set de 2016 at 13:10 Jean Boussier notifications@github.com
wrote:

In spec/rails_spec.rb
#8 (comment):

@@ -52,8 +52,6 @@
png_metadata = ChunkyPNG::Image.from_file(png_path.to_s).metadata
expect(png_metadata).to be_empty

  •  # Metadata usually contains some timestamps, so sleeping is the best way to make sure they are all stripped
    
  •  sleep 1
    

If the metadata are properly stripped, how come this break the build?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/byroot/sprockets-svg/pull/8/files/270cf5d2448c2db2046007e2004736a452cb6099#r78591837,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC66GuShiaa8o3mSTEPM6Xp-BgAKGpOks5qpsrqgaJpZM4J72xp
.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, then it's a problem. Not just because of the flaky test suite, but also because sprockets does require consistent fingerprints.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be fixed now that I removed the tIME chunk that store the last modified information http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put the sleep back then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

I found that when the image is really big the ChunkyPNG gem is slow to
encode/decode the PNG pixels. As we only want to strip the metadata we
can work in the Datastream that doesn't transform the pixels.

With my test image this method went from 10s to 0.01s.
@byroot
Copy link
Owner

byroot commented Sep 13, 2016

Thanks. Feel free to merge on green, you have commit access IIRC.

@rafaelfranca
Copy link
Contributor Author

Oh. I don't think I'm part of the byroot core team yet 😄

@byroot
Copy link
Owner

byroot commented Sep 13, 2016

Oh really? That can be arranged

@byroot byroot merged commit a14154f into byroot:master Sep 13, 2016
@rafaelfranca rafaelfranca deleted the fast-png-strip branch September 13, 2016 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants