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

Support copying monochrome YUV image. #39

Conversation

ledyba-z
Copy link
Contributor

Hi! I' m working at Link-U, a company that publishes digital comics for smart phones.

When monochrome images are decoded by dav1d, U and V planes will be NULL.

However, avifImageCopy assumes that all Y, U and V planes are filled with non-NULL values if and only if the image has YUV planes:

libavif/src/avif.c

Lines 152 to 154 in c541cf9

if (srcImage->yuvPlanes[AVIF_CHAN_Y] && srcImage->yuvPlanes[AVIF_CHAN_U] && srcImage->yuvPlanes[AVIF_CHAN_V]) {
avifImageAllocatePlanes(dstImage, AVIF_PLANES_YUV);

Thus, when avifDecoderRead reads a monochrome image (that has only Y plane), the last call of avifImageCopy does not copy at all, and all planes in the returned copied image will be NULL.

libavif/src/read.c

Lines 1924 to 1939 in c541cf9

avifResult avifDecoderRead(avifDecoder * decoder, avifImage * image, avifROData * input)
{
avifResult result = avifDecoderParse(decoder, input);
if (result != AVIF_RESULT_OK) {
return result;
}
result = avifDecoderNextImage(decoder);
if (result != AVIF_RESULT_OK) {
return result;
}
if (!decoder->image) {
return AVIF_RESULT_NO_IMAGES_REMAINING;
}
avifImageCopy(image, decoder->image);
return AVIF_RESULT_OK;
}

In this patch, just check Y plane to determine an image has YUV planes, and free U, V planes if they are unused.

Please take a look.

P.S.

I also have an idea adding AVIF_PLANES_Y enum to avifPlanesFlags, to tell avifImageAllocatePlanes to allocate just only Y plane. How about this idea?

@joedrago
Copy link
Collaborator

I'm currently unavailable to properly fix the copy issue until Tuesday, but if you are trying to avoid unnecessary allocations and copying on decode, I highly recommend using the "advanced" decoding example in the README. It only ends up being one more function call, and it directly gives you the Y plane allocated by dav1d in decoder->image.

If you use this path instead, you should no longer have the copy issue (which I will fix next week), and you will save 3 allocations and copies.

@ledyba-z
Copy link
Contributor Author

Thanks you for telling me that "advanced" decoding way.
I will try that in SDWebImageAVIFCoder, the project I'm currently working on.

I am going to wait your fix, so I made a separated Issue about this:
#40

And I will close this issue!

@ledyba-z ledyba-z closed this Jan 20, 2020
@ledyba-z ledyba-z deleted the fix/support-copy-monochrome-yuv-image branch January 20, 2020 09:45
@dreampiggy
Copy link

dreampiggy commented Jan 20, 2020

@ledyba-z I’m the code owner of SDWebImageAVIFCoder. If we can, choose the advanced decoding API is OK

PR is welcomed. I’ll release a new version with Swift Package Manager support, if you can implements the advanced API, let me merge and release.

Yesterday I spend much time on porting the libavif(https://github.com/SDWebImage/libavif-Xcode), libaom, libdav1d with SwiftPM support :( That’s a hard and boring work....So I need a little time to relax.

Or if this is harder to implements, I can release a minor version firstly, then another minor version with you PR

@dreampiggy
Copy link

dreampiggy commented Jan 20, 2020

@joedrago Hi, for iOS user, developer prefers to use the Swift Package Manager dependency tool, not CMake. I maintain a wrapper with SwiftPM support. But everytine you release a new version, I have to update the git submodule and tag a new version...This work is a little tired. Can SwiftPM be officially supported ?

If you want to support, just need one file named Package.swift. It’s a DSL to declare compile unit, not hard to learn. I’ve already done with 0.4.8 libavif, you can have a reference to this : https://github.com/SDWebImage/libavif-Xcode/blob/master/Package.swift.

Or even, if you want, I can create a PR to implements SwiftPM, even the future maintain of Travis-CI to ensure SwiftPM build works. SwiftPM can works for Linux/macOS as well.

@ledyba-z
Copy link
Contributor Author

@dreampiggy
It is easy to use advanced decoding API. I already added to my wip branch:

link-u/SDWebImageAVIFCoder@7ae9c81

When using this advanced API, we can't call avifImageYUVToRGB (it crashes).
So I am going to include this advanded decoding API patch to SDWebImage/SDWebImageAVIFCoder#7 . Please wait a moment!

@ledyba-z ledyba-z restored the fix/support-copy-monochrome-yuv-image branch January 20, 2020 21:07
@dreampiggy
Copy link

dreampiggy commented Jan 21, 2020

@ledyba-z Today I up-to-date with the CocoaPods's libavif to the latest release v0.5.3. Fix the potential issue related to libavif itself (The last time I updated was 0.4.4 I remember). Try pod update again on your local branch for SDWebImageAVIFCoder demo to debug.

@ledyba-z
Copy link
Contributor Author

@dreampiggy Thank you for your effort!
Unfortunately, this issue is not solved in v.0.5.3 yet, but I made the pull request to use advanced decoding way.

Please take a look: SDWebImage/SDWebImageAVIFCoder#7

@ledyba-z ledyba-z deleted the fix/support-copy-monochrome-yuv-image branch February 15, 2020 14:54
wantehchang pushed a commit that referenced this pull request Dec 17, 2020
Fix some clang warnings

- Disable `-Wused-but-marked-unused` on Clang on MINGW. This solves #435.
  I created an issue but didn't get response from MINGW maintainers for a week, so for now just disable the warning in this case.

- Remove extra semicolon in `codec_svt.c`. This fixes `-Wextra-semi-stmt` warning.
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

3 participants