Open djhoese opened 1 year ago
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:
SIFT
on the bottom right) - gif, mp4 and m4v have a correct footer/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)
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
Also:
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?
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.
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.
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.
For my future reference:
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.
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.
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.
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?
Ok the missing colorbar and dataset name when you select "Current" is due to the changes in this commit:
https://github.com/ssec/sift/commit/3a74993693cffc7b49bc151e75fa78eeabb03f33
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.
Indeed, the videos are always 480x640
I wonder if it was always that way...should we fix that (the videos being that size)?
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.
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.
Size issue: https://github.com/imageio/imageio/issues/1009
Basically my hacky "divide by 2" filter I added reveals what I consider a bug in imageio/pyav.
@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.
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:
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 thispyav
plugin usage when saving animations so that we don't accidentally use the old plugin or have issues with other plugins being used.This needs heavy testing.