LibGfx+Tests(+AK): Add the beginning of a JPEG2000 loader #23682

nico · 2024-03-23T00:28:18Z

JPEG2000 is the last image format used in PDF filters that we don't have a loader for. Let's change that.

JPEG2000 in practice come in an ISOBMFF container, so most commits in here teach our ISOBMFF code about JPEG2000 types. Our isobmff tool can now print JPEG2000 types. Before:

% Build/lagom/bin/isobmff ~/Desktop/demo.jp2                                    
Unknown Box ('jP  ')
[ 4 bytes ]
('ftyp') (version = 0, flags = 0x0)
- major_brand = 'jp2 '
- minor_version = 0
- compatible_brands = { 'jp2 ' }
Unknown Box ('jp2h')
[ 3211 bytes ]
Unknown Box ('jp2c')
[ 6399 bytes ]

After (this is the same file):

 % Build/lagom/bin/isobmff Tests/LibGfx/test-inputs/jpeg2000/simple.jp2 
('jP  ')
- signature = 0x0d0a870a
('ftyp')
- major_brand = 'jp2 '
- minor_version = 0
- compatible_brands = { 'jp2 ' }
('jp2h')
  ('ihdr')
  - height = 101
  - width = 119
  - num_components = 4
  - are_components_signed = false
  - bits_per_component = 8
  - compression_type = 7
  - is_colorspace_unknown = 1
  - contains_intellectual_property_rights = 0
  ('colr')
  - method = 2
  - precedence = 0
  - approximation = 0
  - icc_data = 3144 bytes
  ('cdef')
  - channel_index = 0
  - channel_type = 0 (color)
  - channel_association = 1
  - channel_index = 3
  - channel_type = 1 (opacity)
  - channel_association = 0
  - channel_index = 1
  - channel_type = 0 (color)
  - channel_association = 2
  - channel_index = 2
  - channel_type = 0 (color)
  - channel_association = 3
('jp2c')
- codestream = 6399 bytes

Furthermore:

% Build/lagom/bin/file  Tests/LibGfx/test-inputs/jpeg2000/simple.jp2            
Tests/LibGfx/test-inputs/jpeg2000/simple.jp2: JPEG2000 image data, 119 x 101

% Build/lagom/bin/icc Tests/LibGfx/test-inputs/jpeg2000/simple.jp2 | grep ascii:
    ascii: "sRGB IEC61966-2.1"
    ascii: "IEC http://www.iec.ch"
    ascii: "IEC 61966-2.1 Default RGB colour space - sRGB"
    ascii: "Reference Viewing Condition in IEC61966-2.1"

nico · 2024-03-23T00:28:50Z

@Zaggy1024 because it's mostly ISOBMFF.

LucasChollet

I left a comment about a small nit. I only had a glance, but I haven't seen anything too shocking 😄

Userland/Libraries/LibGfx/ImageFormats/JPEG2000Loader.cpp

timschumi

Looking good for the most part, just a few nitpicks.

Userland/Libraries/LibGfx/ImageFormats/ISOBMFF/Enums.h

AK/MaybeOwned.h

Userland/Libraries/LibGfx/ImageFormats/ISOBMFF/JPEG2000Boxes.cpp

Userland/Libraries/LibGfx/ImageFormats/JPEG2000Loader.cpp

Tests/LibGfx/TestImageDecoder.cpp

Userland/Libraries/LibGfx/ImageFormats/ISOBMFF/JPEG2000Boxes.cpp

Userland/Libraries/LibGfx/ImageFormats/ISOBMFF/Reader.cpp

Userland/Libraries/LibGfx/ImageFormats/JPEG2000Loader.cpp

nico · 2024-03-23T14:12:39Z

All addressed, ready for another look!

Userland/Libraries/LibGfx/ImageFormats/JPEG2000Loader.cpp

Userland/Libraries/LibGfx/ImageFormats/ISOBMFF/Enums.h

timschumi · 2024-03-24T10:11:17Z

AK/MaybeOwned.h

+    template<typename F>
+    friend class MaybeOwned;
+
+    template<class U, class D>
+    Variant<NonnullOwnPtr<D>, D*>
+    downcast(Variant<NonnullOwnPtr<U>, U*>&& variant)
+    {
+        if (variant.template has<U*>())
+            return variant.template get<U*>();
+        return static_cast<NonnullOwnPtr<T>&&>(move(variant.template get<NonnullOwnPtr<U>>()));
+    }
+
    Variant<NonnullOwnPtr<T>, T*> m_handle;


It seems that at some point we could really use a

template<typename T> using Handle = Variant<NonnullOwnPtr<T>, T*>;

(Review is still pending, this is just something that I thought of randomly.)

You mean generally, not in this PR, yes?

(Should maybe be V<NNOP, WeakPtr> if we want to make it a whole thing.)

It would be nice if at least the type alias is part of this commit, since we are adding two more usages of that type here. However, it's neither urgent nor strictly required, so I'll leave the decision to you.

Making the raw pointer a WeakPtr requires making the stored object RefCounted, which is something that we can't do anyways.

timschumi

Last two small things (one new thing and one reopened conversation).

Userland/Libraries/LibGfx/ImageFormats/JPEG2000Loader.cpp

timschumi · 2024-03-25T17:27:21Z

AK/MaybeOwned.h

+    template<typename F>
+    friend class MaybeOwned;
+
+    template<class U, class D>
+    Variant<NonnullOwnPtr<D>, D*>
+    downcast(Variant<NonnullOwnPtr<U>, U*>&& variant)
+    {
+        if (variant.template has<U*>())
+            return variant.template get<U*>();
+        return static_cast<NonnullOwnPtr<T>&&>(move(variant.template get<NonnullOwnPtr<U>>()));
+    }
+
    Variant<NonnullOwnPtr<T>, T*> m_handle;


It would be nice if at least the type alias is part of this commit, since we are adding two more usages of that type here. However, it's neither urgent nor strictly required, so I'll leave the decision to you.

Making the raw pointer a WeakPtr requires making the stored object RefCounted, which is something that we can't do anyways.

This allows types that have spaces in their FourCC.

I prefixed the types that are labeled as "JPEG2000" on https://mp4ra.org/registered-types/boxes with "JPEG2000".

nico

Thanks, all done.

Userland/Libraries/LibGfx/ImageFormats/JPEG2000Loader.cpp

nico · 2024-03-25T17:37:43Z

(Also threw in 14b9a8e since I'm touching this file anyways.)

timschumi · 2024-03-25T17:50:04Z

(Also threw in 14b9a8e since I'm touching this file anyways.)

For what it's worth, this "design" was kind of intentional on my part, since each branch is equally "important" (compared to, let's say, an early return or error case).

nico · 2024-03-25T18:18:43Z

I don't mind rebasing but clangd also flags it, so I'm not sure what in your message has precedence :)

(I always build the cfg in my head somehow, and an if gets a cfg block and an after-if block but an if/else gets an if block, an else block, and an after-both block, but then both if and else blocks end with jumps out of the function and the poor after-if block ends up empty and unused and sad and was like "you made me get out of bed for this 😢" and "what was I made for 😔" etc etc)

...and make Reader always have a BoxStream.

`isobmff` can now dump the id in a JPEG2000SignatureBox. Creates JPEG2000Boxes.{h,cpp} to house JPEG2000 box types.

...and make SuperBox a pure superclass that's not usable by itself.

This doesn't have to be a virtual method: it's called from various create_from_stream() methods that have a static type that's created. There's no point in the virtual call here, and it makes it harder to add additional parameters to read_from_stream() in some subclasses.

This will allow creating different child boxes in different containers.

JPEG2000 is the last image format used in PDF filters that we don't have a loader for. Let's change that. This adds all the scaffolding, but no actual implementation yet.

This is enough for `file` to print the dimensions of .jp2 / .jpx files, and for `icc` to print color profile information embedded in the 'colr' box.

We can't decode any actual image data yet, but it shows that we can read the basics of the container format. (...as long as there's an Annex I container around the data, not just an Annex A codestream. All files I've found so far have the container.) I drew the thes input in Acorn.app and used "Save as..." to save it as JPEG2000. It's an RGBA image.

nico · 2024-03-25T18:42:20Z

I dropped the else removal and put in an else in my new function. I do think it looks very weird, but I'd also rather have this move forward than argue about this :)

nico requested a review from timschumi as a code owner March 23, 2024 00:28

github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Mar 23, 2024

nico force-pushed the like-its-19-90-9 branch 4 times, most recently from 7501fa8 to efdccde Compare March 23, 2024 01:28

LucasChollet reviewed Mar 23, 2024

View reviewed changes

Userland/Libraries/LibGfx/ImageFormats/JPEG2000Loader.cpp Show resolved Hide resolved

timschumi requested changes Mar 23, 2024

View reviewed changes

nico force-pushed the like-its-19-90-9 branch from 970082f to 6a860bc Compare March 23, 2024 14:12

nico requested review from timschumi and LucasChollet March 23, 2024 14:12

Zaggy1024 reviewed Mar 23, 2024

View reviewed changes

Userland/Libraries/LibGfx/ImageFormats/JPEG2000Loader.cpp Outdated Show resolved Hide resolved

Userland/Libraries/LibGfx/ImageFormats/ISOBMFF/Enums.h Outdated Show resolved Hide resolved

nico force-pushed the like-its-19-90-9 branch from 6a860bc to 26528ad Compare March 24, 2024 00:24

timschumi reviewed Mar 24, 2024

View reviewed changes

nico requested a review from timschumi March 25, 2024 16:17

timschumi reviewed Mar 25, 2024

View reviewed changes

nico added 5 commits March 25, 2024 13:29

LibGfx/ISOBMFF: FileTypeBox is not a FullBox

9bc3c14

LibGfx/ISOBMFF: Remove prototypes for nonexistent methods

26ffe32

LibGfx/ISOBMFF: Put string literals in box type ENUMERATE_ONE()

215ed4e

This allows types that have spaces in their FourCC.

LibGfx/ISOBMFF: Alphabetize box type ENUMERATE_ONE() lines

ef2a11f

LibGfx/ISOBMFF: Add JPEG2000 box types

1973cdd

I prefixed the types that are labeled as "JPEG2000" on https://mp4ra.org/registered-types/boxes with "JPEG2000".

nico force-pushed the like-its-19-90-9 branch from 26528ad to e1d3b40 Compare March 25, 2024 17:36

nico commented Mar 25, 2024

View reviewed changes

Userland/Libraries/LibGfx/ImageFormats/JPEG2000Loader.cpp Outdated Show resolved Hide resolved

timschumi approved these changes Mar 25, 2024

View reviewed changes

timschumi added ✅ pr-maintainer-approved-but-awaiting-ci PR has been approved by a maintainer and can be merged after CI has passed and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels Mar 25, 2024

nico added 20 commits March 25, 2024 14:41

AK: Allow creating a MaybeOwned<Superclass> from a MaybeOwned<Subclass>

1cba87a

LibGfx/ISOBMFF: Make BoxStream MaybeOwn its stream

ab6140e

...and make Reader always have a BoxStream.

LibGfx/ISOBMFF: Read JPEG2000HeaderBox

defb3cf

LibGfx/ISOBMFF: Start creating JPEG2000 box types

0879bb3

`isobmff` can now dump the id in a JPEG2000SignatureBox. Creates JPEG2000Boxes.{h,cpp} to house JPEG2000 box types.

LibGfx/ISOBMFF: Give JPEG2000HeaderBox its own type

c1c93c5

...and make SuperBox a pure superclass that's not usable by itself.

LibGfx/ISOBMFF: Give Reader::read_entire_file() a factory callback

f988eae

This will allow creating different child boxes in different containers.

LibGfx/ISOBMFF: Add JPEG2000ImageHeaderBox

145bc64

LibGfx/ISOBMFF: Add JPEG2000ColorSpecificationBox

808c1ad

LibGfx/ISOBMFF: Add JPEG2000UUIDInfoBox

2a47c10

LibGfx/ISOBMFF: Add JPEG2000ResolutionBox

84867f5

LibGfx/ISOBMFF: Add JPEG2000CaptureResolutionBox

b13303b

LibGfx/ISOBMFF: Add JPEG2000UUIDListBox

e69fd09

LibGfx/ISOBMFF: Add JPEG2000ContiguousCodestreamBox

1fbd38b

LibGfx/ISOBMFF: Add JPEG2000URLBox

40430c0

LibGfx/ISOBMFF: Add JPEG2000ChannelDefinitionBox

eadccbd

LibGfx: Add the start of a JPEG2000 loader

59bb15c

JPEG2000 is the last image format used in PDF filters that we don't have a loader for. Let's change that. This adds all the scaffolding, but no actual implementation yet.

LibGfx/JPEG2000: Read file structure

1e0cb1b

This is enough for `file` to print the dimensions of .jp2 / .jpx files, and for `icc` to print color profile information embedded in the 'colr' box.

Fuzzers: Add JPEG2000 fuzzer

ef853c9

nico force-pushed the like-its-19-90-9 branch from e1d3b40 to ef853c9 Compare March 25, 2024 18:41

github-actions bot added 👀 pr-needs-review PR needs review from a maintainer or community member and removed ✅ pr-maintainer-approved-but-awaiting-ci PR has been approved by a maintainer and can be merged after CI has passed labels Mar 25, 2024

timschumi added ✅ pr-maintainer-approved-but-awaiting-ci PR has been approved by a maintainer and can be merged after CI has passed and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels Mar 25, 2024

timschumi merged commit 2e14210 into SerenityOS:master Mar 25, 2024
7 checks passed

github-actions bot removed the ✅ pr-maintainer-approved-but-awaiting-ci PR has been approved by a maintainer and can be merged after CI has passed label Mar 25, 2024

nico deleted the like-its-19-90-9 branch March 25, 2024 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LibGfx+Tests(+AK): Add the beginning of a JPEG2000 loader #23682

LibGfx+Tests(+AK): Add the beginning of a JPEG2000 loader #23682

nico commented Mar 23, 2024

nico commented Mar 23, 2024

LucasChollet left a comment

timschumi left a comment

nico commented Mar 23, 2024

timschumi Mar 24, 2024 •

edited

nico Mar 25, 2024

timschumi Mar 25, 2024 •

edited

timschumi left a comment

timschumi Mar 25, 2024 •

edited

nico left a comment

nico commented Mar 25, 2024

timschumi commented Mar 25, 2024

nico commented Mar 25, 2024

nico commented Mar 25, 2024

LibGfx+Tests(+AK): Add the beginning of a JPEG2000 loader #23682

LibGfx+Tests(+AK): Add the beginning of a JPEG2000 loader #23682

Conversation

nico commented Mar 23, 2024

nico commented Mar 23, 2024

LucasChollet left a comment

Choose a reason for hiding this comment

timschumi left a comment

Choose a reason for hiding this comment

nico commented Mar 23, 2024

timschumi Mar 24, 2024 • edited

Choose a reason for hiding this comment

nico Mar 25, 2024

Choose a reason for hiding this comment

timschumi Mar 25, 2024 • edited

Choose a reason for hiding this comment

timschumi left a comment

Choose a reason for hiding this comment

timschumi Mar 25, 2024 • edited

Choose a reason for hiding this comment

nico left a comment

Choose a reason for hiding this comment

nico commented Mar 25, 2024

timschumi commented Mar 25, 2024

nico commented Mar 25, 2024

nico commented Mar 25, 2024

timschumi Mar 24, 2024 •

edited

timschumi Mar 25, 2024 •

edited

timschumi Mar 25, 2024 •

edited