ApeironTsuka / node-webpmux

A mostly 1:1 re-implementation of webpmux as a Node module in pure Javascript. Only thing currently missing is a command-line version.
GNU Lesser General Public License v3.0
21 stars 8 forks source link

fix muxAnim function #2

Closed mpirescarvalho closed 3 years ago

mpirescarvalho commented 3 years ago

This PR fixes problems with setting exif data to animated webp files. It's basically some missing/wrong params.

ApeironTsuka commented 3 years ago

The reason for that line is that if a chunk's size is odd, there's an extra padding byte at the end that must be skipped over. Or at least that's how it's supposed to work. I might've misunderstood and the rule doesn't apply in nested chunks (such as in the case of animations). I'll add this change with a couple others.

mpirescarvalho commented 3 years ago

It didn't work very well, the image lost its width and height.

I think passing this.width and this.height is necessary: image

Also, I don't think setting exif data worked, but I'm out of time and haven't tested it enough so it could be something on my end. I'll run some tests tomorow and post here the results

mpirescarvalho commented 3 years ago

By the way, reading ANMF chunk worked

ApeironTsuka commented 3 years ago

Should be fixed properly now. I hadn't noticed line 299 (cursor += header.size+1;) had that +1 before that should not have been there. Reading ANMF chunks should work properly now. Writing ANMF chunks should also now work correctly.

As for .muxAnim not having width/height set, that's because frames could have been added/removed/changed that would then change the animation's final dimensions. Not passing width/height causes Image.muxAnim to recalculate them. I added width and height overrides to .muxAnim in case it's needed/desired.

For saving EXIF, remember that you must tell .muxAnim that you want it saved. img.muxAnim({ path: 'output.webp', exif: true, loops: img.anim.loopCount }) for instance. This passes img's EXIF to Image.muxAnim.

mpirescarvalho commented 3 years ago

I'm sorry, I've got it wrong!

In fact my width and height are identical for both the input and output image. It turns out it's the background that is wrong. Take a look at this:

The source image has a transparent background image

But it is outputing a black background image

I've tried playing arround with param bgColor in muxAnim() but it didn't make any difference

Edit: transparent background works fine for non animated files (using save() function)

By the way, the exif data worked perfectly

ApeironTsuka commented 3 years ago

Pushed what should be a fix. It wasn't reading ALPH chunks at all as for some reason I only did a codepath for if the ALPH chunk came after VP8, even though the "correct" case is ALPH coming before it.

mpirescarvalho commented 3 years ago

It didn't work, still losing transparency 😕

I'm using it like this:

  const img = new NodeWebP.Image();
  await img.load('./video.webp');
  const exif = fs.readFileSync('./exif.exif');
  img.exif = exif
  await img.muxAnim({ path: './sticker.webp', exif: true })

Result is the same as my previous comment

ApeironTsuka commented 3 years ago

Pushed an update changing how .muxAnim works a little bit. It will now use the image's already-defined background color, loop count, width/height, and automatically pass EXIF/ICCP/XMP when available. This is how it should've been working all along, honestly. No longer need to pass exif: true, and instead would need to pass exif: false if you don't want it preserved.

I think what went on this time is it was using the default (should be white) background in the output, maybe? I did some testing with animations that were using ALPH+VP8 (lossy with alpha) and VP8L (lossless with alpha) frames on transparent backgrounds, and both came out as expected this time.

I haven't been pushing these last handful of changes to NPM. Now they're up as 1.1.5, just in case.

mpirescarvalho commented 3 years ago

I haven't been pushing these last handful of changes to NPM. Now they're up as 1.1.5, just in case.

I know, I'm pulling from this repo

It is still outputing a black background. I can push the source and the output to a repo if you want to take a look at the files.

ApeironTsuka commented 3 years ago

That would help, if you can. I'm not sure what's going on now.

mpirescarvalho commented 3 years ago

Just uploaded: input.webp and output.webp

https://github.com/mpirescarvalho/node-webpmux-test

ApeironTsuka commented 3 years ago

Definitely fixed now, thanks. Hopefully you don't run into any more issues. I definitely need to put together a large suite of test images to throw at this later, as my existing testing has clearly not been enough.

mpirescarvalho commented 3 years ago

it's working now, thanks!

edit: you forgot to push to npm 😅

ApeironTsuka commented 3 years ago

Was waiting in case something else cropped up. Pushing now.