Skip to content

Resolve Gallery base64 representation issues#2265

Merged
pngwn merged 48 commits into
gradio-app:mainfrom
proxyphi:gallery-b64-format
Oct 7, 2022
Merged

Resolve Gallery base64 representation issues#2265
pngwn merged 48 commits into
gradio-app:mainfrom
proxyphi:gallery-b64-format

Conversation

@proxyphi
Copy link
Copy Markdown
Contributor

Description

Resolves performance issues caused by base64 image representation in gr.Gallery and gr.Image.

Changes include:

  • Updating Gallery.postprocess and Image.postprocess to store and serve tempfiles of images, in-line with how Audio and Video components are implemented
  • Updating Gallery.svelte and Image.svelte UI components to handle FileData.
  • Slight refactor of normalise_file in ui/packages/upload/utils.ts: broke out array case handling into new normalise_files function.
  • Updated test_static to expect filepath instead of base64 encoding.

Opening this as a draft PR since I had been working on it the past few days, and noticed #2262 getting posted, so I thought it might be of use ☺️

It does go beyond the original scope of the issue and serves Images as well, as I had run into severe performance issues on mobile when working with images beyond 1024x1024.

Closes: #2262

Checklist:

  • I have performed a self-review of my own code
  • My code follows the style guidelines of this project
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@abidlabs
Copy link
Copy Markdown
Member

Awesome, thank you so much for the PR @proxyphi! It looks like this is basically ready, right? If so, we can take look at why the tests are failing and fix that up.

Thanks again!

@proxyphi
Copy link
Copy Markdown
Contributor Author

Hello! This is mostly ready, yes. I believe a couple of tests are breaking due to the move from Image and Gallery's postprocess functions now returning a dict ({name, data, is_file}) with tempfile information, instead of a b64 string.

I can only see one test, test_static from test_components breaking when I run tests locally, though it seems like another is breaking on CircleCI from the external tests. Not sure why that is at the moment; I'll have to take a look at that later.

@abidlabs
Copy link
Copy Markdown
Member

Sounds good, thanks for the explanation @proxyphi. Backend changes look good -- I made some minor cleanups to the docstrings.

@aliabid94 would you be able to review the frontend changes?

@abidlabs
Copy link
Copy Markdown
Member

Thanks for fixing the tests @proxyphi! Opening this up for review

@abidlabs abidlabs marked this pull request as ready for review September 15, 2022 19:00
@proxyphi
Copy link
Copy Markdown
Contributor Author

Great! So I believe that one image-related test failing currently in test_external.py has made me aware of another potential issue: cached examples for audio (and other media types, probably) may not work with the external API to my understanding. Tried the following test locally:

image

Result:
image

Comment thread test/test_components.py Outdated
"""
component = gr.Image("test/test_files/bus.png")
self.assertEqual(component.get_config().get("value"), media_data.BASE64_IMAGE)
tmp_filename = component.get_config().get("value").get("name")
Copy link
Copy Markdown
Member

@abidlabs abidlabs Sep 15, 2022

Choose a reason for hiding this comment

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

This is testing the processing_utils.encode_file_to_base64() function rather than the gr.Image.postprocess() function. Instead, we can use file comparisons (filecmp.cmp) to confirm that the postprocessed file is the same as the original file like we do for the Audio and Video components.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The tests in test_examples.py are doing the same

@abidlabs
Copy link
Copy Markdown
Member

@proxyphi testing the PR and right now, Image examples in general are not working for me. Does this work for you:

import gradio as gr

gr.Interface(lambda x:x, "image", "image", examples=["lion.jpg"]).launch()

@aliabid94 aliabid94 self-requested a review September 16, 2022 22:02
* change Image to derive from FileSerializable
* handle potential dict output of Image, Audio, and Video breaking calls to HF API, and also breaking examples
* add several tests and use filecmp where appropriate
* handle FileData input to an input image component on frontend
* ensure FileData input gets converted to b64 before being handed to sketch component
@proxyphi
Copy link
Copy Markdown
Contributor Author

Just went ahead and committed some changes that should resolve that issue - I was running into it also. It was being caused by the serialization of the Image component effectively conflicting with the new output from postprocess.

I've made some changes that should make Image more in line with the other media components, though I ran into some general issues revolving around FileSerializable data being passed through Examples, or when passed to the HF API. Some tests I've added with audio confirmed that it wasn't originally limited to the changes to Image.

These should be mitigated now! But I've gone ahead and marked some TODO's where I felt things may need a closer look 😇

@abidlabs
Copy link
Copy Markdown
Member

Thanks so much for making the fixes @proxyphi! Taking a look now

@pngwn pngwn merged commit a36dcb5 into gradio-app:main Oct 7, 2022
ghost referenced this pull request in AUTOMATIC1111/stable-diffusion-webui Oct 14, 2022
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.

Don't serialize gr.Gallery() images to base64 format

4 participants