feat(libjpeg_turbo): add JPEG image EXIF data parsing #5263

liujp-arch · 2024-01-10T12:41:26Z

Parse the rotation information of a JPEG image

Help us review this PR! Anyone can approve it or request changes.

Description of the feature or fix

A clear and concise description of what the bug or new feature is.

Checkpoints

Update the Documentation if needed.
Add Examples if relevant.
Add Tests if applicable.
If you added new options to lv_conf_template.h run lv_conf_internal_gen.py and update Kconfig.
Run scripts/code-format.py (astyle needs to be installed) and follow the Code Conventions

src/draw/lv_image_buf.h

src/widgets/image/lv_image.c

PGNetHun

Please do not store the rotation EXIF data in LVGL image object, instead the decoder should decode the image in the correct angle.

kisvegabor · 2024-01-11T04:11:33Z

Please do not store the rotation EXIF data in LVGL image object, instead the decoder should decode the image in the correct angle.

I agree.

XuNeo · 2024-01-11T12:22:39Z

Interesting.
What API should we use for decoder to rotate an image(lv_draw_buf_t * decoded)?

PGNetHun · 2024-01-11T13:13:07Z

Interesting. What API should we use for decoder to rotate an image(lv_draw_buf_t * decoded)?

Well, I think we should not use LVGL to rotate an image (if it's stored for example in portrait mode / 90 degree rotation), instead the image should be decoded in the correct angle/rotation based on the rotation info in source image meta/EXIF data.
So, the decoder is responsible to decode in the correct angle.

PGNetHun

To test the code changes, could you please add a small test JPEG file (and the reference PNG file to compare)?
The JPEG file should have that specific EXIF data for which you have created this PR.

tests/ref_imgs/libs/freetype_1.lp64.png

tests/ref_imgs/libs/jpg_2.png

src/libs/libjpeg_turbo/lv_libjpeg_turbo.c

tests/src/test_assets/test_img_lvgl_logo_with_exif.jpg

PGNetHun · 2024-01-15T14:15:32Z

src/libs/libjpeg_turbo/lv_libjpeg_turbo.c

@@ -369,8 +393,10 @@ static lv_draw_buf_t * decode_jpeg_file(const char * filename)
            jpeg_read_scanlines(&cinfo, buffer, 1);

            /* Assume put_scanline_someplace wants a pointer and sample count. */
-            lv_memcpy(cur_pos, buffer[0], stride);
-            cur_pos += decoded->header.stride;
+            dispose_buffer(cur_pos, buffer[0], line_index, cinfo.output_width, cinfo.output_height, decoded->header.stride,


Can you please rename function to rotate_buffer()?

Dispose means to me releasing a resource, cleaning up, for example in .NET we use for that purpose.
But there is also a dispose pattern for releasing resources, so dispose is not the best name for rotating image parts.

PGNetHun · 2024-01-15T14:17:21Z

src/libs/libjpeg_turbo/lv_libjpeg_turbo.c

@@ -37,16 +38,27 @@ static lv_result_t decoder_open(lv_image_decoder_t * decoder, lv_image_decoder_d
                                const lv_image_decoder_args_t * args);
 static void decoder_close(lv_image_decoder_t * decoder, lv_image_decoder_dsc_t * dsc);
 static lv_draw_buf_t * decode_jpeg_file(const char * filename);
-static bool get_jpeg_size(const char * filename, uint32_t * width, uint32_t * height);
+static uint8_t * alloc_file(const char * filename, uint32_t * size);


Because you are already editing this file, could you please rename alloc_file() to read_file() ?
This name describes better what the function does: reads the file content into memory.

thanks for the feedback, I'll update them later.

tests/src/test_assets/test_img_lvgl_logo_with_exif.jpg

PGNetHun

Only 2 CI builds fail on this libjpeg changes.

PGNetHun

Please fix CI build errors -> libjpeg test fails.
It seems there is a memory leak somehow.

liujp-arch · 2024-01-18T07:33:42Z

looks like need more memory actually.

kisvegabor · 2024-01-18T08:30:26Z

I've checked the test image, found 2 non rotated images, and thought something is not working. And them saw this comment.

I think showing the 180° rotated image as normal is less intuitive.

XuNeo · 2024-01-18T08:58:54Z

If I understand correctly, this PR introduces capability to rotate JPEG image as instructed.
So the better test case would be a 180 rotated logo . Seeing the image is rotated means it works.

PGNetHun · 2024-01-18T09:06:29Z

If I understand correctly, this PR introduces capability to rotate JPEG image as instructed. So the better test case would be a 180 rotated logo . Seeing the image is rotated means it works.

I assume the intention was to show a rotated image correct:
for example a portrait image, which is stored 90 degree rotated in JPEG.

In this PR maybe a label could be added below the 2 images (that show correct, without upside-down):

Not rotated
180 degree rotated

PS:
just to test it, maybe a 90 degree rotated test image could be shown, too.... for bonus points :)

kisvegabor · 2024-01-18T10:06:59Z

Ah,it seems I'm really misunderstanding something. Let's say I take a portrait picture with my camera. I guess the EXIF rotation will be 90°. But if we ignore the EXIF data, what image will be shown? A portrait or a landscape?

liujp-arch · 2024-01-19T03:01:47Z

it‘s actually like this,

when we take a picture with camera(i mean really camera), we may not always hold the camera in such a position that the camera top corresponds to top of the scene, if we hold the camera with orientation flag '6' in above image, the Exif rotation will be 90°. it's equivalent to take a portrait picture with a phone camera. if we ignore the EXIF data, the encoded jpeg will be landscape.

kisvegabor · 2024-01-19T09:30:35Z

All clear, thank you!

If by processing the EXIF data we get exactly the same output as with a normal image, I suggest using this the original reference image for all 4 EXIF options.

lvgl-bot · 2024-02-03T01:45:20Z

We need some feedback on this pull request.

Now we mark this as "stale" because there was no activity here for 14 days.

Remove the "stale" label or comment else this will be closed in 7 days.

terry0012 · 2024-02-04T02:11:58Z

@liujp-arch @kisvegabor
Are there any other points of concern regarding this feature? Can we merge it?

My personal suggestion is to build a set of basic function APIs for image processing in future, such as rotation and scaling, similar to the draw API. This will unify their usage in image decoding and other parts, and also make it easier to accelerate them using SIMD in the future.

PGNetHun · 2024-02-04T16:29:07Z

The actual code is OK (for me), there was only 1 suggestion:
have different input JPEG files with different rotations, but only 1 reference image.
See: #5263 (comment)

But I think we can do it also later, after merging this PR.

terry0012 · 2024-02-05T02:36:11Z

All clear, thank you!

If by processing the EXIF data we get exactly the same output as with a normal image, I suggest using this the original reference image for all 4 EXIF options.

@kisvegabor you mean all the 4 EXIF options images should be decoded to the left image in the last line ?

kisvegabor · 2024-02-07T09:29:32Z

@kisvegabor you mean all the 4 EXIF options images should be decoded to the left image in the last line ?

Yes, we can have only a single reference image like this:

and all 4 variants can be compared to that.

lvgl-bot · 2024-02-25T01:47:05Z

We need some feedback on this pull request.

Now we mark this as "stale" because there was no activity here for 14 days.

Remove the "stale" label or comment else this will be closed in 7 days.

kisvegabor · 2024-02-25T07:01:50Z

I think it's a useful feature and only a little is missing. Did not give up now! 🙂

liujp-arch · 2024-02-28T07:12:27Z

I need to do more testing on the rotation of the image. I will update once it's finished.

Parse the rotation information of a JPEG image

tests/ref_imgs/libs/jpg_2.png

liujp-arch force-pushed the liujp_img branch from 533698e to acb3f89 Compare January 10, 2024 13:05

PGNetHun requested changes Jan 10, 2024

View reviewed changes

src/draw/lv_image_buf.h Outdated Show resolved Hide resolved

src/draw/lv_image_buf.h Outdated Show resolved Hide resolved

src/widgets/image/lv_image.c Outdated Show resolved Hide resolved

PGNetHun requested changes Jan 10, 2024

View reviewed changes

liujp-arch force-pushed the liujp_img branch 6 times, most recently from e44bad4 to d14fbff Compare January 12, 2024 13:33

PGNetHun requested changes Jan 13, 2024

View reviewed changes

tests/ref_imgs/libs/freetype_1.lp64.png Outdated Show resolved Hide resolved

tests/ref_imgs/libs/jpg_2.png Outdated Show resolved Hide resolved

liujp-arch force-pushed the liujp_img branch 4 times, most recently from f8fac45 to 9fa845a Compare January 14, 2024 08:41

kisvegabor requested review from PGNetHun and FASTSHIFT and removed request for PGNetHun January 14, 2024 09:52

PGNetHun requested changes Jan 15, 2024

View reviewed changes

PGNetHun requested changes Jan 16, 2024

View reviewed changes

tests/src/test_assets/test_img_lvgl_logo_with_exif.jpg Outdated Show resolved Hide resolved

liujp-arch force-pushed the liujp_img branch 4 times, most recently from 1173a4a to 2b86afd Compare January 18, 2024 06:24

PGNetHun approved these changes Jan 18, 2024

View reviewed changes

PGNetHun requested changes Jan 18, 2024

View reviewed changes

liujp-arch force-pushed the liujp_img branch from 2b86afd to a44f3dd Compare January 18, 2024 07:13

kisvegabor mentioned this pull request Jan 18, 2024

chore(bin_decoder): make ownership of decompressed memory clear #5369

Merged

W-Mai approved these changes Jan 19, 2024

View reviewed changes

lvgl-bot added the stale label Feb 3, 2024

FASTSHIFT removed the stale label Feb 4, 2024

lvgl-bot added the stale label Feb 25, 2024

FASTSHIFT removed the stale label Feb 26, 2024

liujp-arch dismissed PGNetHun’s stale review via a432b88 February 29, 2024 03:18

liujp-arch force-pushed the liujp_img branch 2 times, most recently from a432b88 to 4066b78 Compare February 29, 2024 03:24

feat: Add JPEG image EXIF data parsing

24aa0fb

Parse the rotation information of a JPEG image

liujp-arch force-pushed the liujp_img branch from 4066b78 to 24aa0fb Compare February 29, 2024 03:30

liujp-arch requested a review from PGNetHun February 29, 2024 06:41

PGNetHun approved these changes Mar 1, 2024

View reviewed changes

tests/ref_imgs/libs/jpg_2.png Show resolved Hide resolved

FASTSHIFT approved these changes Mar 4, 2024

View reviewed changes

FASTSHIFT changed the title ~~feat: Add JPEG image EXIF data parsing~~ feat(libjpeg_turbo): add JPEG image EXIF data parsing Mar 10, 2024

FASTSHIFT merged commit 30793d7 into lvgl:master Mar 10, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(libjpeg_turbo): add JPEG image EXIF data parsing #5263

feat(libjpeg_turbo): add JPEG image EXIF data parsing #5263

liujp-arch commented Jan 10, 2024

PGNetHun left a comment

kisvegabor commented Jan 11, 2024

XuNeo commented Jan 11, 2024

PGNetHun commented Jan 11, 2024 •

edited

Loading

PGNetHun left a comment

PGNetHun Jan 15, 2024

PGNetHun Jan 15, 2024

liujp-arch Jan 16, 2024

PGNetHun left a comment •

edited

Loading

PGNetHun left a comment •

edited

Loading

liujp-arch commented Jan 18, 2024

kisvegabor commented Jan 18, 2024 •

edited

Loading

XuNeo commented Jan 18, 2024

PGNetHun commented Jan 18, 2024 •

edited

Loading

kisvegabor commented Jan 18, 2024

liujp-arch commented Jan 19, 2024 •

edited

Loading

kisvegabor commented Jan 19, 2024

lvgl-bot commented Feb 3, 2024

terry0012 commented Feb 4, 2024

PGNetHun commented Feb 4, 2024

terry0012 commented Feb 5, 2024

kisvegabor commented Feb 7, 2024

lvgl-bot commented Feb 25, 2024

kisvegabor commented Feb 25, 2024

liujp-arch commented Feb 28, 2024

feat(libjpeg_turbo): add JPEG image EXIF data parsing #5263

feat(libjpeg_turbo): add JPEG image EXIF data parsing #5263

Conversation

liujp-arch commented Jan 10, 2024

Description of the feature or fix

Checkpoints

PGNetHun left a comment

Choose a reason for hiding this comment

kisvegabor commented Jan 11, 2024

XuNeo commented Jan 11, 2024

PGNetHun commented Jan 11, 2024 • edited Loading

PGNetHun left a comment

Choose a reason for hiding this comment

PGNetHun Jan 15, 2024

Choose a reason for hiding this comment

PGNetHun Jan 15, 2024

Choose a reason for hiding this comment

liujp-arch Jan 16, 2024

Choose a reason for hiding this comment

PGNetHun left a comment • edited Loading

Choose a reason for hiding this comment

PGNetHun left a comment • edited Loading

Choose a reason for hiding this comment

liujp-arch commented Jan 18, 2024

kisvegabor commented Jan 18, 2024 • edited Loading

XuNeo commented Jan 18, 2024

PGNetHun commented Jan 18, 2024 • edited Loading

kisvegabor commented Jan 18, 2024

liujp-arch commented Jan 19, 2024 • edited Loading

kisvegabor commented Jan 19, 2024

lvgl-bot commented Feb 3, 2024

terry0012 commented Feb 4, 2024

PGNetHun commented Feb 4, 2024

terry0012 commented Feb 5, 2024

kisvegabor commented Feb 7, 2024

lvgl-bot commented Feb 25, 2024

kisvegabor commented Feb 25, 2024

liujp-arch commented Feb 28, 2024

PGNetHun commented Jan 11, 2024 •

edited

Loading

PGNetHun left a comment •

edited

Loading

PGNetHun left a comment •

edited

Loading

kisvegabor commented Jan 18, 2024 •

edited

Loading

PGNetHun commented Jan 18, 2024 •

edited

Loading

liujp-arch commented Jan 19, 2024 •

edited

Loading