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

Issue with color_profile on iOS #397

Open
NikolayJuly opened this issue Dec 9, 2020 · 13 comments
Open

Issue with color_profile on iOS #397

NikolayJuly opened this issue Dec 9, 2020 · 13 comments

Comments

@NikolayJuly
Copy link

Hello. I would like describe an issue, which I run into using this libheif/

Pre story: For few years, I used my mac to generate content for iOS application. Content is heic images. And everything was fine till this BigSur update. Reason is simple: after I updated on BigSur images which i generate on mac, can't be opened in iOS app running on iOS 12. For now both sides used native tools: CGImage, etc

I saw 1 different in heif-info tool response
With old images, generated on catalina - color profile: no
But with BigSur generating color profile: nclx

So I logically assumed that this is an issue. And I tried use this library on macOS instead of native one. I hoped that I will be able to avoid setting random color profile, but this lib also set by default nclx color profile. And no way to ask set nothing.
And I prefer avoid using libheif in iOS app, since it will be simpler switch to jpg instead of heic.

Could you advice the way to not set random/default color profile using this library?

@NikolayJuly NikolayJuly changed the title Issue with color_profile iOS Issue with color_profile on iOS Dec 9, 2020
@farindk
Copy link
Contributor

farindk commented Dec 9, 2020

I don't understand why it should be a problem to have a nclx in the image.
The nclx is no full color-profile (like ICC), it is just defining how to correctly interpret the YCbCr coefficients. Without nclx, this is undefined and the image may look different on different devices.

@NikolayJuly
Copy link
Author

Problem: image which has this color profile can't be read on iOS 12 using native libraries.

@NikolayJuly
Copy link
Author

I'm not saying that this must be implemented, but asking is there way to achieve it?

@silverbacknet
Copy link

Just because it's the only thing that differs in heif-info doesn't mean it's the only difference.

What if you just don't write the colr box and see what happens? That's just adding return Error::Ok; to the beginning of Box_colr::write on line 1855 of box.cc. If that works on iOS 12, then that confirms that the proposed workaround might work, but I have strong doubts that's the actual problem.

@NikolayJuly
Copy link
Author

I will try this, thank you. Will let you a bit later.

@NikolayJuly
Copy link
Author

@silverbacknet I di try this: But resulted image can't be read.
heif-info gave this error: Could not read HEIF file: Invalid input: 'ipma' box references a non-existing property: Nonexisting property (index=6) for item ID=1 referenced in ipma box

To be sure: I actually using last release tag - 1.9.1. Line numbers there different from master. So I found function Error Box_colr::write(StreamWriter& writer) const and return on the first line.

@NikolayJuly
Copy link
Author

I also tried do not write type and just commented writing type and color profile itself.

@farindk
Copy link
Contributor

farindk commented Dec 15, 2020

@NikolayJuly It's probably not enough to add a return Error:Ok to Box_colr::write because (as the error message correctly says), the colr box is not written, but it is still referenced.

You'll have to remove writing the colr box in heif_context.cc::encode_image_as_hevc().
There is a block starting with the comment

  // --- choose which color profile to put into 'colr' box

 if (input_class == heif_image_input_class_normal || input_class == heif_image_input_class_thumbnail) {
    auto icc_profile = image->get_color_profile_icc();
    if (icc_profile) {
      m_heif_file->set_color_profile(image_id, icc_profile);
    }

    if (nclx_profile &&
        (!icc_profile || (options->version >= 3 &&
                          options->save_two_colr_boxes_when_ICC_and_nclx_available))) {
      m_heif_file->set_color_profile(image_id, nclx_profile);
    }
  }

remove this block of code. It might also be enough to remove the second part, which is writing the nclx profile if there is no ICC profile.

I'm a bit reluctant to add an option to switch off writing nclx, because if iOS 12 cannot read nclx profiles, it is obviously a bug in iOS 12 and we should not care too much providing workarounds for old iOS versions.

@NikolayJuly
Copy link
Author

@farindk Sorry for long silence, were busy with xmas prep. I can confirm, that commenting this color profile lines, actually fixed everything. Image can be natively read on iOS 12. Thanks for your help

In separate message I will describe my experience working with libheif and what I believe should be fixed, even tho I assume not everyone will agree with me

@NikolayJuly
Copy link
Author

Thank you a lot for help and response. Since I got so much attention, here is what I found, while tryed to use libheif.
I did few try to use this lib and I found few frustrating things, which make using it really hard.

  1. I assume there is some issue in image format to support odd dimensions in image. I did test - 800x800 works fine, 799x800 - after generating image, preview on macOS can't read image. May be issue might be in some memset or memcpy and internal memory layout
  2. Internal memory layout, I was have to refer to this - https://github.com/SDWebImage/SDWebImageHEIFCoder to find out, that there is some memoty layout tricks, with this magic 16 number, check these lines . I see there is return of strides, but it didn't help actually find out how to do memset. So I would prefer lib accept lines of pixels, as input and write it on its own to proper part of internal memory of a plane. Or even whole image pixel data, and later write it properly line by line properly to a plane.
  3. Looks like there is no any memset when I create a plane, which is strange, I trully believe memory should be initiated, not just allocated, especially since there is some memory layout tricks. Tho I might be wrong, and just didn't find it
  4. Support of native tools and iOS's. This should be open format. It means, if I save image on my mac, I should be able to open it on other systems which support heif. Apple obv failed here. BigSur generate heic images which I can't read on iOS devices. And here your lib was a good option to use. But as we see by default it does the same thing. And when you introduce breaking changes - like start writing some color profiles(which no one actually set), tool to disable it should be provided too. And treat iOS 12 as some old iOS also not right. Because it means, I need use this libheif on both side (my mac and users phone) to make it works, which is not how open image format should work. I should be in control what you writing to image, and be able to make backward compatible image.

farindk added a commit that referenced this issue Jan 7, 2021
@farindk
Copy link
Contributor

farindk commented Jan 7, 2021

@NikolayJuly

  1. I thought the compatibility problem with odd image sizes was fixed in v1.10.0. Please check with that version. If there still is a problem, send me the input image, encoding parameters and tell me on which Apple OS you have tested this.
  2. the image is simply get_height() lines with 'stride' bytes in each. There may be additional padding, but that is irrelevant outside of libheif. If you do a single memset with get_height() * stride, you are fine.
  3. A memset at plane creation takes time and is unnecessary since the application will fill in data anyway.
  4. I have added a encoding flag to workaround the problem that images with nclx profiles cannot be read on macOS (ad6d3c7). It is on by default. Could you please test the current version with this commit and check whether you still have any problems with macOS?

@NikolayJuly
Copy link
Author

NikolayJuly commented Jan 9, 2021

@farindk Thanks a lot for your response.

I thought the compatibility problem with odd image sizes was fixed in v1.10.0. Please check with that version. If there still is a problem, send me the input image, encoding parameters and tell me on which Apple OS you have tested this.

All my test was done on 1.9.1, looks like I missed that 1.10 was released in the middle. But I didn't test 1.10 now as well, I checked out recommended commit, so whole response will be about that commit.
Indeed, now odd images sizes are rounded up to even in result images. I tried 799*799, macOS's preview told me resulted image was 800*800

the image is simply get_height() lines with 'stride' bytes in each. There may be additional padding, but that is irrelevant outside of libheif. If you do a single memset with get_height() * stride, you are fine.

I tested it now and yes, it worked. Might be the case with odd image before, when I did initial tests.

A memset at plane creation takes time and is unnecessary since the application will fill in data anyway.

My initial understanding was that if I do not set all pixels, image can't be encoded. Again, might be pointed to my initial tests with odd image. So, if image can be created with not initialised memory correctly, then yes, it is fine not to set memory. My initial issue was, that to start using lib I actually was have to set all pixels and it wasn't easy, which just making learning curve harder.

I have added a encoding flag to workaround the problem that images with nclx profiles cannot be read on macOS (ad6d3c7). It is on by default. Could you please test the current version with this commit and check whether you still have any problems with macOS?

I tested and... it works! I didn't set any parameters, just copied image data. And it works on iOS 12. And additionally heif-info indeed says color profile: no. Thank you very much for this PR and for your help!

What should I do? Just close it?

1div0 pushed a commit to 1div0/libheif that referenced this issue Feb 7, 2021
eclipseo added a commit to eclipseo/libheif that referenced this issue Jul 21, 2021
@bradh
Copy link
Contributor

bradh commented Dec 24, 2023

I think this got feedback, and can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants