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

Fix export image and switch to imageio v3 API #375

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Jun 13, 2023

Closes #372. This PR got really big really fast. Basically our tests should have caught an incompatibility between our code and updates in imageio. However, things were mocked so heavily in the tests that we weren't even actually using imageio. Things to know:

  1. imageio offers a "v3" sub-package to use the newer version 3 of their API. This seemed like the easiest way to get things working while sort of future-proofing our usage of imageio.
  2. The imageio-ffmpeg package is being deprecated in favor of a new pyav (av on PyPI and conda-forge...I don't know why). This is supposed to be faster, but it comes with some issues. I have to specify a codec (hardcoded to libx624) and specify the ffmpeg filter to scale images that are not power-of-2 sized so that they are. I've also forced this pyav plugin usage when saving animations so that we don't accidentally use the old plugin or have issues with other plugins being used.
  3. Rewrote the export image tests...a lot.

This needs heavy testing.

@djhoese djhoese added bug Something isn't working priority: high labels Jun 13, 2023
@djhoese djhoese added this to the Version 2.0 milestone Jun 13, 2023
@djhoese djhoese self-assigned this Jun 13, 2023
@ameraner
Copy link
Collaborator

Thanks for all your work on this!
I just got the chance to test this properly (I installed av into the environment of the v2.0.0b0 conda-pack, and ran your branch from there).

Some observations:

  1. the exports in principle work again for all output formats, nice! png and jpg for single frames, gif, mp4 and m4v for animations
  2. The png and jpg have an empty footer (apart from SIFT on the bottom right) - gif, mp4 and m4v have a correct footer
  3. The png and jpg do not have a colorbar when requested - but it appears ok on gif, mp4, m4v
  4. The mp4 and m4v appear a bit "squeezed" in the horizontal dimension
  5. After some fiddling, I got this - maybe we need to close the created figures?
/tcenas/home/andream/src/sift/uwsift/view/export_image.py:254: RuntimeWarning: More than 20 figures have been opened. Figures created through the pyplot interface (`matplotlib.pyplot.figure`) are retained until explicitly closed and may consume too much memory. (To control this warning, see the rcParam `figure.max_open_warning`). Consider using `matplotlib.pyplot.close()`.
  fig = plt.figure(figsize=(size[0] / dpi * 0.1, size[1] / dpi * 1.2), dpi=dpi)
  1. When saving to gif and selecting the Frame Delay as constant or fps, I got ( pillow version is 9.5.0):
Traceback (most recent call last):
  File "/tcenas/home/andream/src/sift/uwsift/view/export_image.py", line 447, in _save_screenshot
    self._write_images(filenames, params)
  File "/tcenas/home/andream/src/sift/uwsift/view/export_image.py", line 453, in _write_images
    imageio.imwrite(filename, images_arrays, **params)
  File "/tcenas/proj/optcalimg/OFTs/SIFT/conda-packaged_v2.0.0b0/SIFT_2.0.0b0/lib/python3.11/site-packages/imageio/v3.py", line 147, in imwrite
    encoded = img_file.write(image, **kwargs)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tcenas/proj/optcalimg/OFTs/SIFT/conda-packaged_v2.0.0b0/SIFT_2.0.0b0/lib/python3.11/site-packages/imageio/plugins/pillow.py", line 354, in write
    raise TypeError(
TypeError: The keyword `fps` is no longer supported. Use `duration`(in ms) instead, e.g. `fps=50` == `duration=20` (1000 * 1/50).

Whether you want to tackle (some of) the issues above in this PR, or split them in new issues to tackle separately, it's up to you of course.

Below are an example image for jpg with missing footer and colorbar, and a nice working gif version. I have put the "squeezed" mp4 example here: https://sftp.eumetsat.int/public/file/kjoiQVc_lkucpfRbgn_A6A/irtest.m4v

irtest
irtest

@ameraner
Copy link
Collaborator

Also:
8. It doesn't seem that the png is being transparent where there is no data - the image is the same as for jpg, with black background.

Additional note: I just noticed that point 2 and 3 are magically solved when, instead of selecting Frame Range -> "Current", I select "All" or "Frames from/to". See below. Maybe
9) we could also increase the dpi of the outputs, to have the footer and colorbar less pixelated?
image

@djhoese
Copy link
Member Author

djhoese commented Jun 14, 2023

So for 4, that is likely caused by the window/canvas not being a nice power of 2. This gets down to limitations of the mp4 format. I think instead of scaling the images, I could maybe add black bars to the top and right side of the images. Thoughts?

For 8, my guess is we're setting the background color in a way that transparent doesn't actually mean transparent through the whole image. That, or adding a colorbar/footer is setting a black background for the whole image. You could try no colorbar and no footer and see if transparency comes back.

I can look at some of these easier to solve issues.

@djhoese
Copy link
Member Author

djhoese commented Jun 14, 2023

Turns out I wasn't testing the case where fps was not None. I'm adding this to the tests and it does get the same TypeError that you got. I'll hopefully fix that tonight. I really need to work on other projects, but also want to get this very basic/critical feature fixed.

@djhoese
Copy link
Member Author

djhoese commented Jun 15, 2023

Ok so with pytest tests only, I think I've fixed all fps/duration issues/errors/warnings. I'm hoping that that magically fixes your other footer and colorbar issues you were seeing. I also should have fixed the matplotlib close issue. I still need to think about the animation squeezing/stretching. Do we want to add extra black pixels to make ffmpeg happy? Oh maybe I can see what was done in the old ffmpeg plugin.

@djhoese
Copy link
Member Author

djhoese commented Jun 15, 2023

For my future reference:

https://github.com/imageio/imageio-ffmpeg/blob/2f3403a0c3d1a34b208c4a7d9396035479f90838/imageio_ffmpeg/_io.py#L552-L570

Edit: I realize I misunderstood (assumed the wrong thing about) the warning message. I thought it was a power of 2, but it is actually supposed to be divisible by 16. I'm not sure if that helps or hurts the scaling.

@ameraner
Copy link
Collaborator

Regarding 4), black bars sounds like a reasonable solution, if possible!
Indeed the errors are gone now, and fps/duration saving works fine, thanks!
Footer issues on single images (2/3) are still there, tho, unfortunately.
Also saving without footer/colorbar does not activate transparency. But I would say it's a minor issue in the end.

@djhoese
Copy link
Member Author

djhoese commented Jun 15, 2023

We must be setting a black background then. I don't think we can expect to have transparency on the map and I'm not sure how useful it is in most cases.

So I updated the scaling operations to be aligned with 16 pixels which matches what the old ffmpeg plugin did. But the error I was getting was indeed that it wasn't divisible by 2 so I think my old way should have been "decent". Meaning, unless I'm thinking something wrong, it should have been only scaling to 1 pixel less in each direction. Now it will add up to 15 pixels in either direction to make it divisible by 16. I'm hoping the expanded scaling rather than reduction will keep the videos looking OK.

I'll take a quick look at the footer thing.

@djhoese
Copy link
Member Author

djhoese commented Jun 15, 2023

Oh I should have also said, I noticed that any ffmpeg output is expanded to 480x640 no matter the input array size. Can you tell what size the videos you're getting are?

@djhoese
Copy link
Member Author

djhoese commented Jun 16, 2023

Ok the missing colorbar and dataset name when you select "Current" is due to the changes in this commit:

3a74993

The problem is that the scene canvas is treating frame_range is None as "there's no way there is data loaded" when it is supposed to mean "take a screenshot of the current display/frame". So it is then telling the export image code "there aren't any layers loaded" (UUID is ""). I have to look into what get_current_frame_index() returns when no data is loaded.

@ameraner
Copy link
Collaborator

Indeed, the videos are always 480x640

@djhoese
Copy link
Member Author

djhoese commented Jun 19, 2023

I wonder if it was always that way...should we fix that (the videos being that size)?

@ameraner
Copy link
Collaborator

Hmm I found an mp4 file I created in January, where the size is different (752x1136), and also the image is not distorted... so I think it has been working.
I think we definitely should fix it, as it's probably the root cause of the distortion, and also it reduces the quality of the videos unnecessarily.

@djhoese
Copy link
Member Author

djhoese commented Jun 19, 2023

Right now I have a fix for the missing footer and colorbar information, but I can't properly test it because the tests explicitly avoid calling the real SceneGraphManager and therefore the real screenshot method. I'll have to see how hard it is to unmock these things. This would likely be very good for future updates to the code and coverage since then the SGM would be only lightly mocked.

@djhoese
Copy link
Member Author

djhoese commented Jun 19, 2023

Size issue: imageio/imageio#1009

Basically my hacky "divide by 2" filter I added reveals what I consider a bug in imageio/pyav.

@djhoese
Copy link
Member Author

djhoese commented Aug 23, 2023

@ameraner If you want this should be in a good state for testing. I think I've worked around the imageio v3 bug where it was always saving a small video file by just cutting off 1 row or 1 column if necessary instead of doing the ffmpeg filter. If you say this looks good then I think we can merge it...I think. Oh maybe I should look at some of the last fixes I made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Export image"-functionality does not know how to deal with .jpg/.png formats?
2 participants