Skip to content

Conversation

melissawm
Copy link
Member

@melissawm melissawm commented Dec 6, 2023

References and relevant issues

Closes #60 and napari/napari-sphinx-theme#140

Description

Adds mp4 and png fallback to all webm videos. Also moves them to the appropriate folder for html builds. The downside here is that, in addition to size (because we have more files now), the notebook view of the guides can't seem to be able to play the videos (or use the fallback png). I don't think this is something we use often (open the .md files as notebooks) but ideally I'd like to keep it working.

EDIT:
Some of these videos are outdated. For the moment, I only converted them but will follow up with updates before 0.5 Videos have been updated as part of #125

  • Since the fallback videos are now being created on deployment, the only caveat left is the notebook observation above.

@melissawm melissawm added the task label Dec 6, 2023
@melissawm melissawm added this to the 0.5.0 milestone Dec 6, 2023
@github-actions github-actions bot added documentation Improvements or additions to documentation and removed task labels Dec 6, 2023
@psobolewskiPhD psobolewskiPhD self-requested a review December 6, 2023 18:20
@psobolewskiPhD psobolewskiPhD added the enhancement New feature or request label Dec 6, 2023
@psobolewskiPhD
Copy link
Member

Huh, why did CircleCI action not run?

@psobolewskiPhD
Copy link
Member

This fixes https://napari.org/stable/tutorials/annotation/annotate_points.html on my phone! (Safari)
🎉

Copy link
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

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

I checked everything on my mac and phone. Two videos don't work--I think it's a path thing.
I'm not sure how to check the static image?

@Czaki
Copy link
Contributor

Czaki commented Dec 19, 2023

How expensive will be built them during build steep instead of storing a few versions in a repository?

@melissawm
Copy link
Member Author

The pngs are manually generated for now. The mp4s come from a ffmpeg conversion step so potentially could be automated. This could also only be done on deployment instead of every commit.

@Czaki
Copy link
Contributor

Czaki commented Dec 19, 2023

This could also only be done on deployment instead of every commit.

we may also run this when someone add/modify video content

@psobolewskiPhD
Copy link
Member

Gah, this has conflicts with the big layer PR, which also had videos.
I think those videos are all the correct ones, but the trick to get them to load from this PR is still needed?

@melissawm
Copy link
Member Author

It's expected, I can fix those, I've already created the alternate versions when updating the layer videos.

@melissawm
Copy link
Member Author

I believe I am going to take @Czaki's suggestion here and make this a deployment step. Marking as draft in the meantime

@melissawm melissawm marked this pull request as draft February 6, 2024 17:25
@melissawm melissawm force-pushed the webm-fallback branch 2 times, most recently from 58b4253 to 5b9853b Compare February 7, 2024 14:50
@github-actions github-actions bot added the task label Feb 7, 2024
@melissawm melissawm marked this pull request as ready for review February 7, 2024 14:51
@melissawm
Copy link
Member Author

This should be good to go now!

Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

Nice! @Czaki just want to make sure you're ok with this now before merging. But ❤️!

@melissawm I guess we can make a follow-up issue for notebooks. It's kinda weird that it doesn't work, do you have an ideas about why?

Czaki
Czaki previously requested changes Feb 8, 2024
Copy link
Contributor

@Czaki Czaki left a comment

Choose a reason for hiding this comment

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

The PR description is confusing.
It mentions adding mp4 files, but not mp4 files are added.
The second part suggests that it is intentionally because mp4 files will be generatd during deployment. But there is no rule in .gitignore to ignore *.mp4 files so we are fragile on accidental upload.

I think that PR description need to be updated. *.mp4 should be added to .gitignore.

We also need to think how teduce size of commits to napari/napari.github.io as each commit will be huge now. But we could do this later.

@jni
Copy link
Member

jni commented Feb 8, 2024

We also need to think how teduce size of commits to napari/napari.github.io as each commit will be huge now. But we could do this later.

Well, I think commits reuse blobs when binary files match, so I don't think the size of the snapshot will be multiplied by the number of commits. We also overwrite dev each time, so the only thing we need to do is reuse large files between releases. We could indeed come up with a solution for that. However I think our priority should rather be to move to autogenerated videos...

@Czaki
Copy link
Contributor

Czaki commented Feb 8, 2024

Well, I think commits reuse blobs when binary files match

Yes. But part of the binary of video are metadata which contains the creation date. And dates will be different, so binary blobs will be different.

We also overwrite dev each time

No. We are adding a new version of binary files each time. This is why this repository is so huge.

@melissawm
Copy link
Member Author

Do we need to? Or is rebasing to keep the history lighter an option?

I'm not sure how to solve this issue of the fallbacks otherwise...

@Czaki
Copy link
Contributor

Czaki commented Feb 8, 2024

Do we need to? Or is rebasing to keep the history lighter an option?

I'm not sure how to solve this issue of the fallbacks otherwise...

I am not convinced that we need to solve this in this PR, as it will require significant changes in deploy workflows. I only highlight potential problems. I think that we should start squashing gh-pages branch until we find a proper solution.

@melissawm
Copy link
Member Author

Ok -this should be what we needed. Let me know if there's any improvements I can do. Thanks!

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Mar 8, 2024

Awesome! I get what's going on here, but probably we need either some comments or better yet a note in the docs?

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Mar 10, 2024

Also closes napari/napari-sphinx-theme#140

@melissawm
Copy link
Member Author

melissawm commented Mar 15, 2024

Also moves them to the appropriate folder for html builds. Fallback videos are generated at the deploy docs step.
accompanied by complete and descriptive alt-text. If you're using arrows/circles to highlight portions of the image, make sure
they contrast well against the background of the image.

`````{note} Adding videos
Copy link
Member

@psobolewskiPhD psobolewskiPhD Mar 15, 2024

Choose a reason for hiding this comment

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

It's not letting me suggest, but the title isn't rendering.
image

I think this is because of how notes are handled.
I think this should be:

{admonition} Adding videos
:class: note

or maybe better yet:

{important} Adding videos

Based on https://myst-parser.readthedocs.io/en/latest/syntax/roles-and-directives.html#directives-a-block-level-extension-point

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right, nice catch!

@psobolewskiPhD
Copy link
Member

Tiny nitpick otherwise good to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation videos invisible or hard to play after move to webm
4 participants