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

Png chunk reader codec bitmap #2149

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

mgood7123
Copy link
Contributor

@mgood7123 mgood7123 commented Jul 7, 2022

Description of Change

expose more api's

Bugs Fixed

None

API Changes

Added:

  • bool SKBitmap.SetInfo(SKImageInfo info)
  • bool SKBitmap.SetInfo(SKImageInfo info, int rowBytes)
  • bool SKBitmap.ComputeIsOpaque()
  • void SKBitmap.AllocPixels(SKImageInfo info)
  • void SKBitmap.AllocPixels(SKImageInfo info, int rowBytes)
  • void SKBitmap.AllocPixels(SKImageInfo info, SKBitmapAllocFlags flags)
  • SKImage SKBitmap.AsImage ()
  • uint SKBitmap.GenerationId()
  • SKPixmap SkBitmap.Pixmap { get; }
  • bool SKBitmap.ReadPixels (SKImageInfo info, IntPtr dstpixels, int rowBytes, int x, int y)
  • bool SKBitmap.ReadPixels(SKPixmap dstPixmap)
  • bool SKBitmap.ReadPixels(SKPixmap pixmap, int x, int y)
  • bool SKBitmap.WritePixels(SKPixmap pixmap)
  • bool SKBitmap.WritePixels(SKPixmap dstPixmap, int x, int y)
  • enum SKCodec.SelectionPolicy.preferStillImage
  • enum SKCodec.SelectionPolicy.preferAnimation
  • SKSizeI SKCodec.Dimensions { get; }
  • static SKCodec SKCodec.Create(string filename, SKPngChunkReader chunkReader)
  • static SKCodec SKCodec.Create(string filename, SelectionPolicy selectionPolicy)
  • static SKCodec SKCodec.Create(string filename, SKPngChunkReader chunkReader, SelectionPolicy selectionPolicy)
  • static SKCodec SKCodec.Create(string filename, out SKCodecResult result, SKPngChunkReader chunkReader)
  • static SKCodec SKCodec.Create(string filename, out SKCodecResult result, SelectionPolicy selectionPolicy)
  • static SKCodec SKCodec.Create (string filename, out SKCodecResult result, SKPngChunkReader chunkReader, SelectionPolicy selectionPolicy)
  • static SKCodec SKCodec.Create(Stream stream, SKPngChunkReader chunkReader)
  • static SKCodec SKCodec.Create(Stream stream, SelectionPolicy selectionPolicy)
  • static SKCodec SKCodec.Create(Stream stream, SKPngChunkReader chunkReader, SelectionPolicy selectionPolicy)
  • static SKCodec SKCodec.Create(Stream stream, out SKCodecResult result, SKPngChunkReader chunkReader)
  • static SKCodec SKCodec.Create(Stream stream, out SKCodecResult result, SelectionPolicy selectionPolicy)
  • static SKCodec SKCodec.Create(Stream stream, out SKCodecResult result, SKPngChunkReader chunkReader, SelectionPolicy selectionPolicy)
  • static SKCodec SKCodec.Create(SKStream stream, SKPngChunkReader chunkReader)
  • static SKCodec SKCodec.Create(SKStream stream, SelectionPolicy selectionPolicy)
  • static SKCodec SKCodec.Create(SKStream stream, SKPngChunkReader chunkReader, SelectionPolicy selectionPolicy)
  • static SKCodec SKCodec.Create(SKStream stream, out SKCodecResult result)
  • static SKCodec SKCodec.Create(SKStream stream, out SKCodecResult result, SKPngChunkReader chunkReader)
  • static SKCodec SKCodec.Create(SKStream stream, out SKCodecResult result, SelectionPolicy selectionPolicy)
  • static SKCodec SKCodec.Create(SKStream stream, out SKCodecResult result, SKPngChunkReader chunkReader, SelectionPolicy selectionPolicy)
  • static SKCodec SKCodec.Create(SKData data, SKPngChunkReader chunkReader)
  • abstract bool SKPngChunkReader.ReadChunk(string tag, void* data, IntPtr length)
  • virtual bool SKPngChunkReader.ReadChunk (string tag, IntPtr data, IntPtr length)
  • virtual bool SKPngChunkReader.ReadChunk (void* tag, void* data, IntPtr length)

Behavioral Changes

None.

Required skia PR

mono/skia#88

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Merged related skia PRs
  • Changes adhere to coding standard
  • Updated documentation

@mgood7123 mgood7123 mentioned this pull request Jul 8, 2022
5 tasks
Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

All we need are some API tweaks and then some tests! The main areas for tests are around the pixmap values depending on whether bitmaps have pixel data or not. Also we need tests for various permutations of the chunk reader with some png files with actual tags in it.

binding/Binding/SKCodec.cs Outdated Show resolved Hide resolved
binding/Binding/SKCodec.cs Outdated Show resolved Hide resolved
binding/Binding/SKBitmap.cs Show resolved Hide resolved
binding/Binding/SKBitmap.cs Outdated Show resolved Hide resolved
binding/Binding/SKPngChunkReader.cs Outdated Show resolved Hide resolved
binding/Binding/SKPngChunkReader.cs Outdated Show resolved Hide resolved
binding/Binding/SKPngChunkReader.cs Outdated Show resolved Hide resolved
Comment on lines 498 to 509
public void CanReadPngChunks_iTXt()
{
var path = Path.Combine(PathToImages, "png_chunks/good_itxt.png");

using var chunkReader = new TestPngChunkReader();
using var codec = SKCodec.Create(path, chunkReader, SKCodecSelectionPolicy.PreferStillImage, out var codecResult);
using var bmp = SKBitmap.Decode(codec);

var chunks = chunkReader.Chunks;

Assert.NotEmpty(chunks);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is failing because chunks are not being read still.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you try the following?

internal class Reader : SKPngChunkReader
{
    public bool r = false;

    protected override bool ReadChunk(string tag, IntPtr data, IntPtr length)
    {
        r = true;
        return true;
    }
}
public void CanInvokePngReadChunks(
{
    using Reader chunkReader = new();

    var path = Path.Combine(PathToImages, "png_chunks/good_itxt.png");

    using var codec = SKCodec.Create(path, chunkReader, SKCodecSelectionPolicy.PreferStillImage, out var codecResult);

    Assert.True(r.r, "failed to invoke chunk reader");
}

with SkiaSharp\externals\skia\BUILD.gn

optional("png_decode") {
  enabled = skia_use_libpng_decode
  public_defines = [ "SK_CODEC_DECODES_PNG", "PNG_READ_UNKNOWN_CHUNKS_SUPPORTED" ]

  deps = [ "//third_party/libpng" ]
  sources = [
    "src/codec/SkIcoCodec.cpp",
    "src/codec/SkPngCodec.cpp",
  ]
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already set the flag here #2149 (comment)

I can actually debug and see it reading the chunk but it seems to decide it is not worth it and skips.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://groups.google.com/g/skia-discuss/c/zHVOjwoQ-jQ/m/aKQ-EIZ_AQAJ

it looks like we will need to test it with UNKNOWN chunks instead of KNOWN chunks

Copy link
Contributor Author

@mgood7123 mgood7123 Aug 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coincidentally this [an example implementation here](https://android.googlesource.com/platform/frameworks/base/+/master/libs/hwui/jni/NinePatchPeeker.cpp). is exactly what i have ported

https://github.com/mgood7123/AndroidUI/blob/master/AndroidUI/Graphics/NinePatchPeeker.cs

https://github.com/mgood7123/AndroidUI/blob/f7746e01b17bd728f94937da5d96c34c5d6ea024/AndroidUI/Graphics/BitmapFactory.cs#L1254

Copy link
Contributor Author

@mgood7123 mgood7123 Aug 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but i currently dont have any NinePatch images to test with

however the NinePatch (api itself) tests all pass https://github.com/mgood7123/android_9_patch/blob/31931f9910f428207ee482a808cc09195a985c87/9patch_tests.cpp

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can borrow some images from android in the GitHub.com/aosp_mirror

Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgood7123 I see chunking wasn't working in 2 ways, so I opened this discussion: https://groups.google.com/g/skia-discuss/c/zHVOjwoQ-jQ

@mattleibow
Copy link
Contributor

PNG chunk images are from https://www.nayuki.io/page/png-file-chunk-inspector

fix string for unable to create instance
@mattleibow
Copy link
Contributor

Based on the google discussion:

I think this only supports unknown chunks - iTXt and pHYs are standard chunks, which are handled directly by libpng.
And

The Skia API doesn't give you a way to get those chunks (or their data), though, and I'm not sure it should.

I am not sure this adds value since there is no way to read those values in the PNG.

@mgood7123
Copy link
Contributor Author

mgood7123 commented Aug 23, 2022

Based on the google discussion:

I think this only supports unknown chunks - iTXt and pHYs are standard chunks, which are handled directly by libpng.
And

The Skia API doesn't give you a way to get those chunks (or their data), though, and I'm not sure it should.

I am not sure this adds value since there is no way to read those values in the PNG.

i think they meant skia does not give you a way to get the standard chunks (nor does libpng unless you manually decode by yourself)

as unknown chunks SHOULD be supported since android used them to store NinePatch data

@mattleibow
Copy link
Contributor

I added a test, so before we merge I think we should add a test proving this works. Are you able to find an image that causes the chunks to be read?

I can't merge unless we know for sure this is working.

@mgood7123
Copy link
Contributor Author

I added a test, so before we merge I think we should add a test proving this works. Are you able to find an image that causes the chunks to be read?

I can't merge unless we know for sure this is working.

ill see what i can find

@mgood7123
Copy link
Contributor Author

i tried it with

var bm = BitmapFactory.decodeFile(Context, image_path_NPatch);
var chunks = bm.getNinePatchChunk();
if (NinePatch.isNinePatchChunk(chunks))
{
    Log.d("NINEPATCH", "IS CHUNK YES");
    //image.setImageDrawable(new NinePatchDrawable(bm, chunks, new Rect(), null));
} else
{
    Log.d("NINEPATCH", "IS CHUNK NO");
}

but i get IS CHUNK NO

additionally i added

        protected override bool ReadChunk(string tag, IntPtr data, IntPtr length)
        {
            // NPatch
            System.Console.WriteLine("READ NINEPATCH CHUNK");

but i dont see it anywhere

@mgood7123
Copy link
Contributor Author

mgood7123 commented Aug 24, 2022

circle 9

valid 9 patch image circle.9.png

@mgood7123
Copy link
Contributor Author

in android this works

Bitmap bm = decodeResource(getResources(), R.drawable.circle); // same as DecodeFile
byte[] ninePatchChunk = bm.getNinePatchChunk();
if (NinePatch.isNinePatchChunk(ninePatchChunk)) {
    image.setImageDrawable(new NinePatchDrawable(bm, ninePatchChunk, new Rect(), null));

@mgood7123
Copy link
Contributor Author

mgood7123 commented Aug 25, 2022

hmmm it seems that Android's AAPT creates the 9-patch chunks itself, and the image does not actually contain these chunks (just the pixel info for these chunks)

luckily we can extract this easily :)

image

@mgood7123
Copy link
Contributor Author

and it can read it :)

image

internal class _10_test_png_chunk_reader : Test
{
    internal class Reader : SKPngChunkReader
    {
        public bool r = false;

        protected override bool ReadChunk(string tag, IntPtr data, IntPtr length)
        {
            r = true;
            Console.WriteLine("READ CHUNK");
            return true;
        }
    }
    public override void Run(TestGroup nullableInstance)
    {
        using Reader r = new();
        using FileStream stream = new("C:\\Users\\AndroidUI\\Desktop\\AndroidUI\\AndroidUITest\\png_chunks\\good_itxt.png", FileMode.Open);
        SKCodecResult result_;
        using SKCodec c = SKCodec.Create(stream, out result_, r);
        Tools.ExpectFalse(r.r, "known chunks are not meant to invoke chunk reader");

        using Reader r2 = new();
        using FileStream np = new("K:/Images/9PNG/circle.9.png", FileMode.Open);
        SKCodecResult result2_;
        using SKCodec c2 = SKCodec.Create(np, out result2_, r2);
        Tools.ExpectTrue(r2.r, "unknown chunks are meant to invoke chunk reader");
    }
}

@mgood7123
Copy link
Contributor Author

image

@mgood7123
Copy link
Contributor Author

so now we can read a 9patch image

circle 9

@mgood7123
Copy link
Contributor Author

mgood7123 commented Aug 25, 2022

also plugging this file into the png chunk inspector gives

image

which is correct

@mattleibow
Copy link
Contributor

Can you add this as a test?

@mgood7123
Copy link
Contributor Author

this should work now :)

@mgood7123
Copy link
Contributor Author

Based on the google discussion:

I think this only supports unknown chunks - iTXt and pHYs are standard chunks, which are handled directly by libpng.
And

The Skia API doesn't give you a way to get those chunks (or their data), though, and I'm not sure it should.

I am not sure this adds value since there is no way to read those values in the PNG.

it seems so

basically...

   chunk_name = png_ptr->chunk_name;

   if (chunk_name == png_IDAT)
   {
        // ...
   }

   if (chunk_name == png_IHDR)
   {
        // ...
   }

   else if (chunk_name == png_IEND)
   {
        // ...
   }

#ifdef PNG_HANDLE_AS_UNKNOWN_SUPPORTED
   else if ((keep = png_chunk_unknown_handling(png_ptr, chunk_name)) != 0)
   {
      PNG_PUSH_SAVE_BUFFER_IF_FULL
      png_handle_unknown(png_ptr, info_ptr, png_ptr->push_length, keep);

      if (chunk_name == png_PLTE)
         png_ptr->mode |= PNG_HAVE_PLTE;
   }
#endif

   else if (chunk_name == png_PLTE)
   {
        // ...
   }

   else if (chunk_name == png_IDAT)
   {
        // ...
      return;
   }

#ifdef PNG_READ_gAMA_SUPPORTED
   else if (png_ptr->chunk_name == png_gAMA)
   {
        // ...
   }

#endif
#ifdef PNG_READ_sBIT_SUPPORTED
   else if (png_ptr->chunk_name == png_sBIT)
   {
        // ...
   }

#endif
#ifdef PNG_READ_cHRM_SUPPORTED
   else if (png_ptr->chunk_name == png_cHRM)
   {
        // ...
   }

#endif
#ifdef PNG_READ_sRGB_SUPPORTED
   else if (chunk_name == png_sRGB)
   {
        // ...
   }

#endif
#ifdef PNG_READ_iCCP_SUPPORTED
   else if (png_ptr->chunk_name == png_iCCP)
   {
        // ...
   }

#endif
#ifdef PNG_READ_sPLT_SUPPORTED
   else if (chunk_name == png_sPLT)
   {
        // ...
   }

#endif
#ifdef PNG_READ_tRNS_SUPPORTED
   else if (chunk_name == png_tRNS)
   {
        // ...
   }

#endif
#ifdef PNG_READ_bKGD_SUPPORTED
   else if (chunk_name == png_bKGD)
   {
        // ...
   }

#endif
#ifdef PNG_READ_hIST_SUPPORTED
   else if (chunk_name == png_hIST)
   {
        // ...
   }

#endif
#ifdef PNG_READ_pHYs_SUPPORTED
   else if (chunk_name == png_pHYs)
   {
        // ...
   }

#endif
#ifdef PNG_READ_oFFs_SUPPORTED
   else if (chunk_name == png_oFFs)
   {
        // ...
   }
#endif

#ifdef PNG_READ_pCAL_SUPPORTED
   else if (chunk_name == png_pCAL)
   {
        // ...
   }

#endif
#ifdef PNG_READ_sCAL_SUPPORTED
   else if (chunk_name == png_sCAL)
   {
        // ...
   }

#endif
#ifdef PNG_READ_tIME_SUPPORTED
   else if (chunk_name == png_tIME)
   {
        // ...
   }

#endif
#ifdef PNG_READ_tEXt_SUPPORTED
   else if (chunk_name == png_tEXt)
   {
        // ...
   }

#endif
#ifdef PNG_READ_zTXt_SUPPORTED
   else if (chunk_name == png_zTXt)
   {
        // ...
   }

#endif
#ifdef PNG_READ_iTXt_SUPPORTED
   else if (chunk_name == png_iTXt)
   {
        // ...
   }
#endif

   else
   {
      PNG_PUSH_SAVE_BUFFER_IF_FULL
      png_handle_unknown(png_ptr, info_ptr, png_ptr->push_length,
          PNG_HANDLE_CHUNK_AS_DEFAULT);
   }

   png_ptr->mode &= ~PNG_HAVE_CHUNK_HEADER;

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

Successfully merging this pull request may close these issues.

2 participants