Codecov Report

Attention: Patch coverage is 93.42561% with 19 lines in your changes missing coverage. Please review.

Project coverage is 92.53%. Comparing base (ba91f96) to head (80ebb4b).
Report is 2 commits behind head on main.

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
+ Coverage   89.62%   92.53%   +2.90%     
==========================================
  Files          12       12              
  Lines         945     1219     +274     
  Branches      187      252      +65     
==========================================
+ Hits          847     1128     +281     
+ Misses         98       91       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

The blend_op image was taken from here.

https://philip.html5.org/tests/apng/tests.html#apng-blend-op-over-on-solid-and-transparent-colours

Unlike regular png images I couldn't find/make 16bit or interlaced apng images. 16 bits are supposed to work because the data decoding uses same functions as for a regular png 16 bit image. Interlaced one, I am not even sure if it needs to be added. On one side the specification doesn't say that apng image can't be interlaced. But on the other side the idea of interlaced apng image is weird and i can't find any examples for it. Should I just drop it and leave it for non-interlaced frames?

It is not practical to have a method that can return different types depending on the input data.
I still think we should have a separate public function for decoding APNG. decodePng could still return the first frame like if it was a normal image (and skip decoding the other frames).

Before refactoring, can you suggest typings something that:

• Don't change the types returned by the existing decodePng function
• Expose a new function (could be named like decodeAnimated or decodeAPNG) with specific shape for the result (not just an array of images).

If we can't find an example interlaced APNG, we can drop it for now.

It is not practical to have a method that can return different types depending on the input data. I still think we should have a separate public function for decoding APNG. decodePng could still return the first frame like if it was a normal image (and skip decoding the other frames).

Before refactoring, can you suggest typings something that:

• Don't change the types returned by the existing decodePng function
• Expose a new function (could be named like decodeAnimated or decodeAPNG) with specific shape for the result (not just an array of images).

Then I guess I shouldn't overcomplicate things. Like you said I can just add a separate decodeApng function which will look like this:

  public decodeApng(): DecodedPng[] {
    checkSignature(this);
    while (!this._end) {
      this.decodeChunk();
    }
    const result = this.decodeApngImage();
    return result;
  }

The issue, however, is with trying to decode a regular PNG as APNG. If we want to skip other frames for regular png instead of decoding them, then we need to create another function like decodeApngChunk which does pretty much the same as decodeChunk but it will have logic on how to process apng-exclusive chunks, which regular decodeChunk will not have. I think it is necessary because whether to skip or to decode apng chunks only depends on which function is called: decode or decodeApng. Is it a valid solution?

Yes, that sounds ok. I don't know if it's doable but decodeApngChunk could maybe delegate to decodeChunks for the chunks it doesn't know about, to avoid repetition.

Still, can you suggest a different return type for the decodeApng function?
As a user, I expect to get general data about the PNG at the top level, and frame-specific data at the frame level. I think it should return an object, with the frames in a array in that object.

Yes, that sounds ok. I don't know if it's doable but decodeApngChunk could maybe delegate to decodeChunks for the chunks it doesn't know about, to avoid repetition.

Still, can you suggest a different return type for the decodeApng function? As a user, I expect to get general data about the PNG at the top level, and frame-specific data at the frame level. I think it should return an object, with the frames in a array in that object.

If I understand you correctly, then I think we can keep the initial DecodedPng type for that. The frames can be stored in a separate field.

export interface DecodedPng {
  width: number;
  height: number;
  data: PngDataArray;
  depth: BitDepth;
  channels: number;
  text: Record<string, string>;
  resolution?: PngResolution;
  palette?: IndexedColors;
  transparency?: Uint16Array;
  iccEmbeddedProfile?: IccEmbeddedProfile;
  frames?: DecodedPngFrame[];
}

The idea was to read the apng frame by frame and- since APNG can store only a part of the frame that actually changes -I wanted to create an array of ready-to-be-encoded PNG images. Even if the frame's data gets decoded as it is, it might not hold much value, because it might not contain the whole image, and its metadata serves to kind of "put the changed data" on the main canvas(1st frame). Thus is the question, how should the decoded frames look like?

For the reference here is what a frame looks like:

export interface DecodedPngFrame {
  sequenceNumber: number;
  width: number;
  height: number;
  xOffset: number;
  yOffset: number;
  delayNumber: number;
  delayDenominator: number;
  disposeOp: number;
  blendOp: number;
  data: Uint8Array;
}

Perhaps we can add another function called decodeApngFrameByFrame which still returns decodedPng[] because that's what the issue to be resolved is about. But we can leave decodeApng as a DecodedPng object, because it will allow us to implement apng encoding as well.

The frames should be returned decoded. I feel like you are confusing what the PNGDecoder class returns with what the decodePng and decodeApng user-facing functions return.
I'm not asking necessarily to change what the internal class does, only what the end user will get.

The frames should be returned decoded. I feel like you are confusing what the PNGDecoder class returns with what the decodePng and decodeApng user-facing functions return. I'm not asking necessarily to change what the internal class does, only what the end user will get.

So, the idea is to have something like this?

interface DecodedPng{
  width: number;
  height: number;
  data: PngDataArray;
  depth: BitDepth;
  channels: number;
  text: Record<string, string>;
  resolution?: PngResolution;
  palette?: IndexedColors;
  transparency?: Uint16Array;
  iccEmbeddedProfile?: IccEmbeddedProfile;
frames?: PngDataArray[]; // Stores the decoded data for each frame
}

Not exactly:

• frames would be mandatory
• frames would still contain objects (with timing information at least)
• no data at the top level

Not exactly:

• frames would be mandatory
• frames would still contain objects (with timing information at least)
• no data at the top level

Yeah, fair enough. If we make a separate function for it, there is no need for a common data type. Then we can make something like this:

interface DecodedApng{
  width: number;
  height: number;
  depth: BitDepth;
  channels: number;
  numberOfFrames:number;
  numberOfReps:number;
  resolution?: PngResolution;
  text: Record<string, string>;
  palette?: IndexedColors;
  transparency?: Uint16Array;
  iccEmbeddedProfile?: IccEmbeddedProfile;
frames: Array<{
  sequenceNumber: number;
  delayNumber: number;
  delayDenominator: number;
  data: Uint8Array;
}>
}

Frames objects will have their own type obviously.