PNG silent upload fail on jpg conversion

[fearlessfrog]

Hi!

We’ve had quite a few reports on PNG’s failing image uploads on our install at v1.9.0.beta15 +27. It seems to only be certain PNG’s and not just based on size. It is particularly unfortunate for us because it now fails on the screenshots of a game the forum is based on, so many users are finding the issue. It has only recently started to happen (my guess would be around the Rails 5 or so update for 1.9, but not sure).

I’ve recreated a test case on try.discourse.com and will try to recreate here.

A test image can be found in here:

https://mudspikefiles.s3.amazonaws.com/PNG_problem.zip

The error log we see seems to indicate it is related to the conversion to JPG:

/var/www/discourse/lib/discourse.rb:27:in `execute_command'
/var/www/discourse/lib/upload_creator.rb:137:in `convert_to_jpeg!'
/var/www/discourse/lib/upload_creator.rb:44:in `block in create_for'
/var/www/discourse/lib/distributed_mutex.rb:21:in `synchronize'
/var/www/discourse/lib/distributed_mutex.rb:5:in `synchronize'
/var/www/discourse/lib/upload_creator.rb:36:in `create_for'
/var/www/discourse/app/controllers/uploads_controller.rb:116:in `create_upload'
/var/www/discourse/app/controllers/uploads_controller.rb:29:in `block in create'
/var/www/discourse/lib/hijack.rb:43:in `instance_eval'
/var/www/discourse/lib/hijack.rb:43:in `block in hijack'
/var/www/discourse/lib/scheduler/defer.rb:74:in `block in do_work'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/rails_multisite-1.1.2/lib/rails_multisite/connection_management.rb:77:in `with_connection'
/var/www/discourse/lib/scheduler/defer.rb:72:in `do_work'
/var/www/discourse/lib/scheduler/defer.rb:61:in `block (2 levels) in start_thread'

Here is a working PNG:

Here is the non-working PNG (you should have the error now in your logs):

Thanks for any help, plus let us know if there is any more info I can provide.

[fearlessfrog]

So looking at the Discourse code and the use of Magick (or a derivative?), an Identify command on the PNG shows that it is struggling to get the metadata which is probably causing the issue e.g:

 identify -verbose Screen_171203_123000.png 
identify: Expected 8 bytes; found 0 bytes `Screen_171203_123000.png' @ warning/png.c/MagickPNGWarningHandler/1832.
identify: Read Exception `Screen_171203_123000.png' @ error/png.c/MagickPNGErrorHandler/1806.
identify: corrupt image `Screen_171203_123000.png' @ error/png.c/ReadPNGImage/4077

If we put the setting ‘png to jpg quality’ to 100 to disable it then we can get the images uploaded. The big downside of that is that we really liked the jpg rendering for the large topics that get posted rendering speed.

[riking (Kane York)]

How are these broken images being created? Can you run them through an image editor to fix them prior to upload?

[fearlessfrog]

They are screenshots generated from Digital Combat Simulator. The subject of a lot of our forum content is this PC sim, plus the community post screenshots (which makes us converting them not possible, as discourse was doing it before this):

DCS Screenshots

I’ve reported the issue at their site, but I do not know if it will be addressed. The format has obviously changed, as we’ve been ok the last 3 years or so. The users are confused, as the PNGs render ok in OSX/Windows, but only fail when uploading to our forum.

[codinghorror (Jeff Atwood)]

Maybe @zogstrip can have a look. Can you provide a place to download the image with the corrupt metadata?

[fearlessfrog]

Thanks Jeff. There’s a link in the OP to a zip with the image in it.

After some further digging, I can confirm this is not a Discourse specific bug. It happens on Twitter uploads as well, and is game/sim screenshot dependent. The images the sim is now creating for screenshots are missing “end of image = IEND” chunk info. I’ve reported it to them and will do the ‘Png to jpg’ setting to 100 as a workaround on our forum.

If anything, I suppose this bug report could be a request to make the Discourse error surface to the user if it finds one? As in, if the upload fails with an imagemagick exception like that then let the end user know that the image was corrupt and the upload is not taking place.

[sam (Sam Saffron)]

@zogstrip looks like we are getting a 500 status code here and the error handler is unable to handle the error cause no json is being sent back.

This is happening cause there is an unhandled exception internally. (uploader should handle all of them and return proper errors)

We should probably add some “safeguard” just in case error handling to the js side of thing, eg: if you don’t have an error message display a simple “something went wrong” - like network went down mid request, random 503 and so on.

[zogstrip (Régis Hanol)]

This is now fixed per

https://github.com/discourse/discourse/commit/f5e170c6b5691612bb4bc8dcf9a327d64f619561