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

Add Support for webp format #6035

Merged
merged 9 commits into from
Feb 18, 2024
Merged

Conversation

vaniisgh
Copy link
Contributor

Adding support to add webp files.
#5612

Copy link

codecov bot commented Dec 13, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (f9d9a58) 83.65% compared to head (8931f9a) 39.11%.

❗ Current head 8931f9a differs from pull request most recent head f8e17bb. Consider uploading reports for the commit f8e17bb to get more accurate results

Files Patch % Lines
panel/pane/image.py 35.71% 9 Missing ⚠️
panel/tests/pane/test_image.py 50.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6035       +/-   ##
===========================================
- Coverage   83.65%   39.11%   -44.55%     
===========================================
  Files         305      305               
  Lines       45518    45519        +1     
===========================================
- Hits        38079    17805    -20274     
- Misses       7439    27714    +20275     
Flag Coverage Δ
ui-tests 39.11% <40.00%> (-0.54%) ⬇️
unitexamples-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@hoxbro
Copy link
Member

hoxbro commented Dec 13, 2023

I have uploaded the tree here: https://assets.holoviz.org/panel/samples/webp_sample.webp

panel/pane/image.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ahuang11 ahuang11 left a comment

Choose a reason for hiding this comment

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

Thanks for adding support for webp! Looks good to me! Just some minor suggestions and if you can add a test similar to the other image tests!

examples/reference/panes/Image.ipynb Outdated Show resolved Hide resolved
@vaniisgh
Copy link
Contributor Author

Thanks for your comments I will just get to it 😄

@vaniisgh
Copy link
Contributor Author

vaniisgh commented Dec 21, 2023

hey, im struggling a bit to get the width and height to be set correctly in imgshp does anyone have any suggestions ?
from what I read I seem to be unpacking it correctly. and the base64 image also has the correct header... not sure what im doing wrong ...

@ahuang11
Copy link
Contributor

Hey @vaniisgh sorry for the late reply!

My naive testing suggest it seems to be working. Can you elaborate on what the remaining issue is?

image

@classmethod
def _imgshape(cls, data):
import struct
b = BytesIO(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can use a context manager here:

with BytesIO(data) as b:
    ...

@ahuang11
Copy link
Contributor

ahuang11 commented Dec 27, 2023

This seems to work for the sample image:

import requests

response = requests.get("https://assets.holoviz.org/panel/samples/webp_sample.webp")
data = response.content

with open("webp_sample.webp", "wb") as f:
    f.write(data)

def get_webp_dimensions(file_path):
    with open(file_path, 'rb') as f:
        header = f.read(30) 
        width = int.from_bytes(header[26:28], byteorder='little')
        height = int.from_bytes(header[28:30], byteorder='little')

    return width, height

get_webp_dimensions("webp_sample.webp")

Returns:
(320, 241)

Which agrees with Mac's preview
image

@vaniisgh
Copy link
Contributor Author

vaniisgh commented Dec 28, 2023

I think the issue only occurs in the test case ? Maybe the image I have created is incorrect... from base64 to webP. maybe I am not reading the header correctly, you seem to have done it a little different. let me just try it like your demonstration.

while asserting the w,h. though I used the same image as the ones used in png, jpeg etc so I think that can't be the problem.

@ahuang11
Copy link
Contributor

Yes maybe the image might be incorrectly created; perhaps you can use another image if that's the case.

@philippjfr
Copy link
Member

Many thanks for your work on this @vaniisgh, really appreciated!

@vaniisgh
Copy link
Contributor Author

Thanks, sorry for the long period of inactivity

@philippjfr
Copy link
Member

Think that was mostly on us tbh.

@philippjfr philippjfr merged commit 313530f into holoviz:main Feb 18, 2024
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants