Initial implementation for FFmpeg support

[Hakerh400]

This is initial implementation of basic FFmpeg support for loading rare and obscure image formats. Summary of this PR:

1. Adds FFmpeg to Travis
2. Adds function setSource in lib/image.js
3. Performs some listener switching and recovers original listeners when FFmpeg exits
4. Adds few more tests to cover reported issues
5. Adds few more images to cover the new tests
6. Modifies fixtures/159-crash1.jpg file to make test captures errors from libjpeg pass

Issues covered

1. "out of memory" error when trying to load a PNG with a dubious gAMA chunk #73 - Fixed, added test
2. Incorrect transparent GIF blending when it's transparent color is not black #116 - ?
3. Greyscale JPEGs are interpreted as color. #117 - Unable to reproduce
4. PNG with cHRM chunk fails to render #122 - Fixed, added test
5. Problem when using images ('Image given has not completed loading') #254 - Missing detailed info (IMO it can be closed)
6. segfault in canvas.streamJPEGSync #297 - Unable to reproduce, seems to be fixed
7. Typed Array and Chunked image decode #378 - It may be resolved by implementing the function from 6. question (see below)
8. CMYK JPEG images are not properly painted #425 - Not resolved - I don't see efficient way to check if JPEG is in CMYK colorspace. The easiest way is to spawn FFprobe and obtain colorspace info, but it still doesn't resolve the problem in case FFprobe (FFmpeg) binaries are not installed. The main problem is that no errors are reported, but the image is not parsed properly
9. WebP encoding support #562 - Fixed, added test
10. Support BMP Image format #953 - Fixed, added test
11. Loading a JPEG with precision set to 12 crashes all processes #1160 - Fixed, added test
12. Black CMYK JEPGs in PDF with MIME data mode #1183 - Similar to 8.
13. loadImage fails with "Invalid SOS parameters for sequential JPEG" #1235 - Fixed (works for the original image)

Also fixes #1244.

Note: images used in the added tests contain the same problems (and without FFmpeg support report the same errors) as the reported images in the corresponding issues. However, these images are not the images that were originally posted in the issues, since the license of the original images is unknown / unclear / unspecified. The added tests contain only images that are licensed under the Creative Commons Zero License, making it easy to avoid any legal problems that may occur in the future.

Some questions

1. Why does Image has public property source that can be accessed directly from user code? Shouldn't it be hidden and accessible only by calling some internal binding function, which is not exported by node-canvas interface?
2. Do onload and onerror listeners need to be called synchronously? This may slow down the process, especially in a multi-threading application.
3. Also, reading file content in Image::SetSource performs unnecessary blocking IO operation, making the thread blocked until the file is loaded. Also, if decoding fails, FFmpeg has to load the file again. Is it a good idea to move it to JS lib, replacing it with asynchronous fs.readFile? Then, even if decoding fails, FFmpeg doesn't need to load the file again - it can use the same buffer.
4. Some old tests assume synchronous onload and onerror calls. It implies that spawnSync must be used instead of spawn (for FFmpeg). Is it a good practice to refactor the existing tests and addapt them for asynchronous calls?
5. This PR adds new images for tests. These images are licensed under the CC0 license. Under what license(s) are other images published? Shouldn't it be mentioned somewhere explicitly (in the readme, for example)?
6. Currently, FFmpeg encodes loaded image as png, then such buffer is decoded by node-canvas. It is unnecessary work. Would it be a good idea to create a new C++ function that takes Buffer as an argument and places it in the Image's image data? That function will allow FFmpeg to encode images into RGBA format, which is a lot faster.

Checklist

• Update the changelog
• Add a test
• Update readme/wiki (users should be aware that FFmpeg may improve image decoding)

[LinusU]

Out traveling at the moment, but some quick answers:

1. Why does Image has public property source that can be accessed directly from user code? Shouldn't it be hidden and accessible only by calling some internal binding function, which is not exported by node-canvas interface?

On the top of my head, I just think that this is legacy. Would be very nice to hide it before 2.0.

2. Do onload and onerror listeners need to be called synchronously? This may slow down the process, especially in a multi-threading application.

No. In fact, it would be better if we always called them asynchronously.

3. Also, reading file content in Image::SetSource performs unnecessary blocking IO operation, making the thread blocked until the file is loaded. Also, if decoding fails, FFmpeg has to load the file again. Is it a good idea to move it to JS lib, replacing it with asynchronous fs.readFile? Then, even if decoding fails, FFmpeg doesn't need to load the file again - it can use the same buffer.

We should absolutely move away from blocking IO. I'm not sure if moving to JS-land is the best approach though, it's possible to do it from C++ also. Might be easier in JS though so that might have merits.

4. Some old tests assume synchronous onload and onerror calls. It implies that spawnSync must be used instead of spawn (for FFmpeg). Is it a good practice to refactor the existing tests and addapt them for asynchronous calls?

Yes!

5. This PR adds new images for tests. These images are licensed under the CC0 license. Under what license(s) are other images published? Shouldn't it be mentioned somewhere explicitly (in the readme, for example)?

I don't know about the existing images, but might be a good idea to create test/fixtures/licenses.md and list each image and the license in?

6. Currently, FFmpeg encodes loaded image as png, then such buffer is decoded by node-canvas. It is unnecessary work. Would it be a good idea to create a new C++ function that takes Buffer as an argument and places it in the Image's image data? That function will allow FFmpeg to encode images into RGBA format, which is a lot faster.

Sounds good 👍 we could also call ffmpeg directly from C++, might be faster.

If you are interested in non-blocking work in C++, this is a very good helper class to use: https://github.com/nodejs/nan/blob/master/doc/asyncworker.md

[LinusU]

Btw. how large is the ffmpeg binary? An idea is to just ship that for Windows, macOS and Linux bundled, and otherwise use the system installed. That way we could probably drop all the other libraries for image decoding? 🤔

[zbjornson]

Fixing (1) I think requires moving the entirety of the src getters/setters to C++, but yes, that would be ideal.

(2) is #1007.

(3) All of the image source setter code needs to be heavily cleaned up:

• I think all formats except PNG read the entire file into memory and then delegate to load[format]FromBuffer. (PNG is a streaming reader, but see next point.) That I/O code can all be de-duped, unless we switch more loaders to streaming ones.
• As you said, all should be made async. This could be done from JS at least in cases where the loader isn't streaming.
• PNG is a streaming loader, but it uses cairo_image_surface_create_from_png_stream, which is a "toy" (their words) and the reason that there are issues with edge-case PNGs. We already link against libpng; that should be used instead. Would fix "out of memory" error when trying to load a PNG with a dubious gAMA chunk #73 and PNG with cHRM chunk fails to render #122.
• We need the commits I linked to in Optional FFmpeg support #1244 (comment) to address CMYK JPEG images are not properly painted #425 and Black CMYK JEPGs in PDF with MIME data mode #1183 (I think?).
• On top of all of that, I'd love to make the encoding and decoding API pluggable so that new formats can be added with an efficient interface (raw pixel access with no byte swapping, no/low-copy where possible).

an idea is to just ship that for Windows, macOS and Linux bundled, and otherwise use the system installed. That way we could probably drop all the other libraries for image decoding?

I was wondering about this too, or at least using avformat.h's API instead of spawning a process. I don't know how big it is, what dependencies it has or how its performance is (e.g. vs. libjpeg-turbo, which is a very heavily optimized lib).

[Hakerh400]

Thanks for the feedback. Seems that the following things remain to be done:

• Hide Image::GetSource and Image::SetSource
• Make onload and onerror always be asynchronous
• Refactor some old tests
• Move image loading to C++, spawn FFmpeg from there, obtain RGBA data
• Create a C++ function for manipulating Image's image data

I'm working on it.

Btw. how large is the ffmpeg binary? An idea is to just ship that

The FFmpeg itself is not that heavy. The main concern are probably performances. FFmpeg takes ~4ms on average to decode/encode 1080p image, but takes ~30ms to spawn FFmpeg process. Libpng and turbojpeg takes approximately the same time to decode an image, so FFmpeg is not slower in that sense.

That way we could probably drop all the other libraries for image decoding?

This is how that idea seems to me:

• Pros
  • Easier to install node-canvas
  • Supports almost every image format that users may want to decode
  • Reduce the size of the current image loading/decoding code
• Cons
  • Maybe a bit slower
  • FFmpeg binary is ~40MB

using avformat.h's API instead of spawning a process.

Sounds interesting. That would definitely be faster than spawning a new process each time. So, what is the final consensus?

• Including FFmpeg binaries, or linking against avformat.h?
• Dropping libpng & turbojpeg, or leaving them and using FFmpeg as fallback?

[chearon]

Sounds like this has brought some attention to things that needed improvement, which is great! I'm still wondering if this is trying to solve something that prebuilds already solves. Using FFmpeg as a fallback is a cool idea but we should also consider that it's more code paths to maintain.

I would vote for linking against avformat.h too, but it would be a lot more complicated. We have to link against an old version of FFmpeg in order to support a wide range of versions, and don't link until we're sure we need it (otherwise canvas crashes as soon as it's loaded if FFmpeg isn't on the system).

I don't think distributing FFmpeg makes a ton of sense since we could just distribute jpeg-turbo etc, which are faster and smaller.

[LinusU]

I don't think distributing FFmpeg makes a ton of sense since we could just distribute jpeg-turbo etc, which are faster and smaller.

I thought that the problem was that jpeg-turbo doesn't support all of the different obscure jpeg-formats that ffmpeg supports? 🤔

[...] we should also consider that it's more code paths to maintain.

Yeah, definitely a negative factor. That's why I started thinking about only using ffmpeg, if that supports all current, and even more, formats that could be cool.

That would probably not work great with spawn though, it would have to be linked...

[zbjornson]

I thought that the problem was that jpeg-turbo doesn't support all of the different obscure jpeg-formats that ffmpeg supports?

#1160 (edge-case JPEG) should work with libjpeg-turbo 9a. I haven't had a chance to dig into why that fails. #1235 (ditto) I also haven't had a chance to look at. #73 and #122 (edge-case PNGs) should be fixed if we move from canavs's toy API to libpng's API.

The others (aside from webp and bmp format) are now closed (#117, #254, #297, #425) or not fixed by ffmpeg (or at least by this PR, #116, #378, #1183), I think. Webp I have a separate module for in node-gfx that, if not for the dependency trouble, could go in core. (Sorta side note, very recent builds of all of the dependencies needed for Windows, including libwebp, are available here: https://github.com/WebKitForWindows/WinCairoRequirements/releases.)

---

BTW the way this PR is structured currently (all in JS), it could trivially go in a separate module that modifies canvas's Image prototype when it loads. I'm sorta hesitant to move away from libjpeg-turbo and libpng...

[chearon]

Agreed.

(Sorta side note, very recent builds of all of the dependencies needed for Windows, including libwebp, are available here: https://github.com/WebKitForWindows/WinCairoRequirements/releases.)

* Except pango

There's also up-to-date builds for Windows for nearly every popular open source library (including libwebp) here or via MinGW + pacman. They're pretty aggressive at updating libraries

[Hakerh400]

I also agree that dropping all current libraries would be too radical move, and that is not how this PR was planned. The main idea here is to allow user to load wide range of different image formats without installing 3rd-party libraries or modules.

The way this PR is structured currently (all in JS), it could trivially go in a separate module

Yes, but loading images should belong to the scope of this module. Do users need to install a new module (or patch the binding script / Image.prototype) in order to load a bmp file?

The ideal solution that I can think about is to literally make FFmpeg optional. It means: provide some variable (flag) that users can modify from JS code, which tells node-canvas how and when FFmpeg can be used. For example:

var {Image, ffmpeg} = require('canvas');
ffmpeg.set('disabled'); // Disable FFmpeg (obscure images will throw error)
ffmpeg.set('default');  // Use FFmpeg as the primary image decoding software
ffmpeg.set('fallback'); // If node-canvas can't load, use FFmpeg

It will be disabled by default. If a user wants to load some special image, they can simply enable FFmpeg. Doesn't need extra modules or dependencies. And FFmpeg should still be installed by a user, separately from node-canvas.

Thank you for insights so far. I'll go with this idea and see how it turns out.

[chearon]

The main idea here is to allow user to load wide range of different image formats without installing 3rd-party libraries or modules.

The wider range of formats is a fair point. I guess browsers do support more formats than we do for drawing as @zbjornson listed in #953 (comment). I haven't seen people ask for more than GIF/JPEG/PNG very often though.

But the second part - don't need to install 3rd-party libraries - sounds like it might be solving the same issue that distributing prebuilt binaries solves.

[LinusU]

BTW the way this PR is structured currently (all in JS), it could trivially go in a separate module that modifies canvas's Image prototype when it loads.

This might be a good approach 👍

The repo could live under node-gfx

[Hakerh400]

Ok, closing for now. I'll open another PR for Image prototype related stuff.