Skip to content

Does Jpeg format need comment marker support? #2067

Description

@JimBobSquarePants

Discussed in #2061

Originally posted by br3aker March 13, 2022
Current jpeg implementation lacks of COM marker support, does it need to support it? Should be easy to implement as this marker is just an array of bytes - itu spec leaves 'interpretation to the application', decoding API should not try to decode it as it can be anything from literal byte array to some weird text encoding.

So it can be a 'good first issue' or I can work on it a bit later.

Activity

  1. added this to the 3.0.0 milestone on Mar 17, 2022
  2. prabhavmehra commented on Oct 24, 2023

    @prabhavmehra

    any updates on this? I was looking to use it from ImageSharp (as I do use it for bunch of other stuff)

  3. JimBobSquarePants commented on Oct 25, 2023

    @JimBobSquarePants
    MemberAuthor

    No changes, happy to accept contributions though!

  4. br3aker commented on Oct 25, 2023

    @br3aker
    Contributor

    I think this place is better for a small guide than that discussion @prabhavmehra.

    First of all, COM marker is already hooked up in the decoding loop here

    The only thing left to do is to actually parse given stream which is skipped right now using this line: stream.Skip(markerContentByteSize);.

    And the final question is where to store parsed results. Results should be saved into JpegMetadata class. AFAIR we decided to implement it as an ICollection<char[]> property backed by a List<char[]>.

    The only thing that bothers me is what should we do if there are no COM markers in given JPEG, leave new property as null or an empty ICollection...

  5. JimBobSquarePants commented on Oct 25, 2023

    @JimBobSquarePants
    MemberAuthor

    How about a nullable Memory<char>?

  6. tocsoft commented on Oct 25, 2023

    @tocsoft
    Member

    I would like to add in here that I feel reading COM markers should be explicitly opt-in. (could be exposed as a new property on JpegDecoderOptions)
    The writing side of things however can be left as implicitly opt-in by the fact we have the metadata set or not.

    I feel it needs to be opt-in for a couple of reasons.

    1. It feels like the usage would be relatively niche for our general users.
    2. We don't want to be penalizing general users/usage having to keep hold of more memory when most users would never want it.
    3. It would now cause larger file outputs as we would start currying the data in & then out for those users that don't even know this thing exists.
  7. br3aker commented on Oct 26, 2023

    @br3aker
    Contributor

    How about a nullable Memory?

    Jpeg can have many COM markers so either List<Memory<char>> or List<char[]> but what's the profit using Memory<T> here? :)

    @tocsoft extra configuration seems fair.

  8. JimBobSquarePants commented on Oct 26, 2023

    @JimBobSquarePants
    MemberAuthor

    Mostly API consistency, we use that type for color pallet pallet metadata.

  9. br3aker commented on Oct 26, 2023

    @br3aker
    Contributor

    Mostly API consistency, we use that type for color pallet pallet metadata.

    Fair enough, I have nothing against nullable ICollection<Memory<char>> then :)

  10. JimBobSquarePants commented on Jul 31, 2024

    @JimBobSquarePants
    MemberAuthor

    This was added in #2641

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions