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

Allow setting animation format from gallery config #1243

Merged
merged 8 commits into from
Apr 30, 2024

Conversation

QuLogic
Copy link
Contributor

@QuLogic QuLogic commented Dec 15, 2023

Matplotlib currently defaults to rcParams['animation.html'] = 'none', which ends up producing jshtml. This is an HTML fragment with base64-encoded PNG frames, and this becomes fairly large. A better option is html5, which produces an mp4 video (though still base64 encoded). The docs do mention you can set the rcParam, but that is reset for every example by sphinx-gallery calling rcdefaults.

As discussed on #280, that reset is intentional, but does mean that you cannot set animation.html globally in a matplotlibrc. But unlike styling, I think setting animation.html in every example is distracting. It is an option for Animation.save, which is called by sphinx-gallery somewhere outside the example, so it's really not a relevant configuration for someone reading the example.

This PR modifies the matplotlib_animations gallery configuration option to accept a (enabled: bool, fmt: str | None) tuple (with the single boolean allowed for backwards-compatibility.) This fmt setting allows picking between jshtml or html5 (or the previous method if set to None.) I have not yet updated the documentation, but will do so if this seems desirable.

This change does mean you get a video with browser controls instead of the frame-by-frame one that Matplotlib provides, but on the other hand, you save a bunch of space. When configured on the tinybuild here, plot_animation.html is 93% smaller, from 823992 to 58214 bytes.

Note: this is based on #1241 and #1242 right now, so I marked it draft.

@QuLogic
Copy link
Contributor Author

QuLogic commented Dec 15, 2023

Note, it is possible to achieve this externally by replacing matplotlib_scraper by one that sets animation.html before calling it (as fortunately, the resetter is called at a different time.):

def matplotlib_html5_scraper(block, block_vars, gallery_conf, **kwargs):
    matplotlib.rcParams['animation.html'] = 'html5'
    return sphinx_gallery.scrapers.matplotlib_scraper(block, block_vars, gallery_conf, **kwargs)

However, I would like to extend the fmt option to allow all video formats allowed by Matplotlib, and this could not be directly achieved externally. Then sphinx-gallery would have to insert a video tag itself, but the upside is the video is no longer embedded. That would remove the base64 encoding, which in the case of plot_animations here would further reduce it to 27708 bytes. Not only does it save space, but it can be loaded by the browser lazily as and when it feels like, instead of being part of the webpage. I have this partially implemented, but just need to get the files into the right place.

@larsoner
Copy link
Contributor

As discussed on #280, that reset is intentional, but does mean that you cannot set animation.html globally in a matplotlibrc

The suggestion in our docs is to use a resetter like:

For example, to reset matplotlib to always use the ggplot style, you could do:

def reset_mpl(gallery_conf, fname):
from matplotlib import style
style.use('ggplot')

For a real-life example we tweak params in MNE-Python:

https://github.com/mne-tools/mne-python/blob/35f0ef65d02af33acf55ba01fa5aa62d8697e117/doc/conf.py#L612

https://github.com/mne-tools/mne-python/blob/35f0ef65d02af33acf55ba01fa5aa62d8697e117/doc/conf.py#L479-L481

I think it's a more general way to tackle this problem. If you agree, do you see a way to make this feature more discoverable in our docs?

@larsoner
Copy link
Contributor

See mne-tools/mne-python#12298 for an example that implements this resetter strategy, seems to work! File size did decrease from 6.8MB to 224KB for us!

@QuLogic
Copy link
Contributor Author

QuLogic commented Dec 15, 2023

I think using the resetter is equivalent to modifying the scraper as I noted above (which I prefer, as the scraper is public, but the resetter isn't)? This won't allow the followup of de-embedding the videos.

@larsoner
Copy link
Contributor

I think using the resetter is equivalent to modifying the scraper as I noted above (which I prefer, as the scraper is public, but the resetter isn't)?

Actually I consider it to be more of the opposite -- resetter is a publicly documented config/end-user-settable thing that came about IIRC for stuff like setting rcParams if you need to after they've been reset by SG. So I'd expect people to use that rather than make their own scraper that wraps the SG one. So maybe they would be functionally equivalent, but conceptually/historically the resetter approach is what I'd expect people to do.

This won't allow the followup of de-embedding the videos.

Ahh I see what you mean. As long as what you implement here is compatible with users having already set plt.rcParams["animation.html"] then now I see why you'd need it. I missed that embedding issue on first read.

@QuLogic
Copy link
Contributor Author

QuLogic commented Dec 16, 2023

Actually I consider it to be more of the opposite -- resetter is a publicly documented config/end-user-settable thing that came about IIRC for stuff like setting rcParams if you need to after they've been reset by SG

I was under the impression that the resetter is replacing what SG resets, and not just run after. If that's not the case, then I agree it is the better choice.

This won't allow the followup of de-embedding the videos.

Ahh I see what you mean. As long as what you implement here is compatible with users having already set plt.rcParams["animation.html"] then now I see why you'd need it. I missed that embedding issue on first read.

I've managed to implement this locally using docutils's support for video. However, it doesn't support everything that the video tag does (namely, you can set controls, but not autoplay or loop). I can do the work to override that (piggybacking on the image-sg directive), but I wonder if that's something that's wanted? If it's unclear what I mean (which is probably true), I'll push what I have here after my other PR is merged.

@larsoner
Copy link
Contributor

I was under the impression that the resetter is replacing what SG resets, and not just run after.

Yeah you can replace or run after (or before!) depending on how you set the params

I've managed to implement this locally using docutils's support for video. However, it doesn't support everything ... I can do the work to override that

+1 from me. The srcset stuff seems to have worked well since it was implemented and I think docutils just added support for it but it took a while. So I don't mind moving faster here to make things work for people who it would help (like mpl) and trying to upstream things to docutils when we can.

@QuLogic
Copy link
Contributor Author

QuLogic commented Dec 22, 2023

I've posted what I have so you can see the issues I see with the docutils video support on the generated page:

  1. It supports controls, but not autoplay or loop; I don't know how strongly that's something one would want.
  2. It adds a text link after the video with a link to it, but using the file path as the text. I believe you can override it with the alt text on the image, but that's still a bit weird layout-wise.
  3. it doesn't work with PDF output, since it tries to embed the video. Previously animations just got ignored, I think? It would be nice to insert the first frame at least (actually PDF does support video embedding but I'm not sure the current way to do it in LaTeX).

For those reasons I think it might be best to use image-sg or a new video-sg directive that tweaks these little things, assuming they are wanted?

Also, I'm not sure where to add ffmpeg to the test CI.

@larsoner
Copy link
Contributor

Agreed a new directive would be good

For testing you can probably add it here

https://github.com/sphinx-gallery/sphinx-gallery/blob/master/continuous_integration/install.sh#L43

and/or in the mamba stuff above there. But we also don't want to require ffmpeg be installed for everyone all the time to run tests so some pytest.mark.skipif would be good based on whether or not ffmpeg is present. Maybe matplotlib already has some utility for checking this so you don't have to do subprocess stuff manually?

@QuLogic
Copy link
Contributor Author

QuLogic commented Feb 16, 2024

I started writing a new directive, before thinking maybe someone has done this before, and so I found sphinxcontrib-video. It is currently optional unless you've asked for external files, but I could make it required or an extra (it's only a single Python file, so not that heavy of a dependency) if you prefer that.

Unfortunately, I also needed to do some patching there, so this depends on my branch for sphinx-contrib/video#36, but this will hopefully be temporary.

@QuLogic
Copy link
Contributor Author

QuLogic commented Mar 12, 2024

I rebased now that sphinxcontrib-video has a release out; it still fails on runs that don't have ffmpeg installed. But this is in a "full" test file. Do you think I should make a whole new test directory for it, or require ffmpeg everywhere?

@larsoner
Copy link
Contributor

Do you think I should make a whole new test directory for it, or require ffmpeg everywhere?

How about:

  1. Add a pytest.skip if there is no ffmpeg (not sure the best way to check, maybe matplotlib has a helper for this?)
  2. Add system ffmpeg to the Ubuntu pip runs so those don't have to skip

Then the skip will only be hit on Windows pip which I guess is okay. Bonus territory would be adding a windows conda job so that it's not skipped, but I can add that later if needed

@QuLogic QuLogic force-pushed the anim-options branch 3 times, most recently from 6f0dcd4 to 21f490c Compare March 16, 2024 09:38
@QuLogic QuLogic marked this pull request as ready for review March 16, 2024 09:45
@QuLogic
Copy link
Contributor Author

QuLogic commented Mar 27, 2024

Not sure if you got pinged when this was marked ready, but it should be good to go now.

Copy link
Contributor

@lucyleeow lucyleeow left a comment

Choose a reason for hiding this comment

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

Just a flyby as I'm not so familiar with animation.

Will take another look once there are docs! Thanks!

sphinx_gallery/gen_gallery.py Outdated Show resolved Hide resolved
sphinx_gallery/gen_gallery.py Outdated Show resolved Hide resolved
if dpi == "figure":
dpi = fig.dpi
video_uri = video.relative_to(gallery_conf["src_dir"]).as_posix()
html = _ANIMATION_VIDEO_RST.format(
Copy link
Contributor

Choose a reason for hiding this comment

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

Question, in what cases would we use _ANIMATION_VIDEO_RST instead of _ANIMATION_RST, i.e., when is html None ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

html is None if fmt not in (None, "html5", "jshtml"); those are the originally supported formats kept for backwards compatibility.

This will then place them external to the HTML file, saving the extra
base64 encoding.
@QuLogic
Copy link
Contributor Author

QuLogic commented Apr 4, 2024

I rebased, updated for your suggestions, and added some docs.

@lucyleeow
Copy link
Contributor

For my understanding:

  • matplotlib_animations: True effectively means matplotlib_animations: (True, None)
  • matplotlib_animations: (True, None) - means use what users have set plt.rcParams["animation.html"], noting that this will be reset for every code block
  • Setting format in matplotlib_animations sets it globally for all code blocks, but can be over-ridden by setting plt.rcParams["animation.html"] in a code block
  • if format is not None, 'html5' or 'jshtml', the video is saved, not embedded thus reducing size, we use the video directive from sphinxcontrib-video

is this right?

This extension supports more of the HTML video tag's options, allowing
for things like looping, autoplay, etc.
@QuLogic
Copy link
Contributor Author

QuLogic commented Apr 9, 2024

is this right?

Yes, I believe that to be correct.

Feel free to push any followups; I'll be on vacation.

@lucyleeow
Copy link
Contributor

Failures unrelated and I due to new sphinx release, see sphinx-doc/sphinx#12299

@lucyleeow
Copy link
Contributor

Will merge in a few days, in case @larsoner wants to check.

@larsoner
Copy link
Contributor

Looks great, thanks @QuLogic for working on this and @lucyleeow for reviewing !

@larsoner larsoner merged commit fe7bea1 into sphinx-gallery:master Apr 30, 2024
17 checks passed
@QuLogic QuLogic deleted the anim-options branch April 30, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants