Skip to content

Update Playing Videos page for Godot 4.5 #10972

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

berarma
Copy link
Contributor

@berarma berarma commented May 28, 2025

Removed another limitation: change playback speed.

Added Shutter Encoder as a FFmpeg GUI. Related to #10942.

I've decided to remove the date from the FFmpeg warning since they have already released a point version but without the fix. And I don't know how many more they will release yet. I hoped that the next major version was closer to being released.

Changed the GOP description a bit to hopefully better fit the rest of the documentation.

@skyace65
Copy link
Contributor

What specifically in this PR only applies to 4.5?

@berarma
Copy link
Contributor Author

berarma commented May 29, 2025

What specifically in this PR only applies to 4.5?

I've removed some limitations. I've taken the opportunity to rework some of the text I did previously, and reference Shutter Encoder.

@skyace65
Copy link
Contributor

skyace65 commented Jun 8, 2025

Got it. This needs to be rebased since I just merged #10721. Also is there a reason this is marked as a draft?

@berarma
Copy link
Contributor Author

berarma commented Jun 8, 2025

Oh, sorry, I had forgot about that PR. There's another limitation to remove. I'll do it in this PR.

It's marked as a draft because one of the new features wasn't yet merged. Now that it is I'll update this PR.

The reason I've reworked some of my previous additions to this page is that I feel a bit insecure writing documentation. When I read some parts of it, it feels too verbose or more intricate than necessary. Your judgement on this would help me. Thanks.

@berarma berarma force-pushed the playing_videos_4.5 branch 2 times, most recently from 0d450ef to 3abcd3b Compare June 9, 2025 11:39
@berarma berarma marked this pull request as ready for review June 9, 2025 11:47
@berarma
Copy link
Contributor Author

berarma commented Jun 9, 2025

Description updated.

@AThousandShips AThousandShips added area:manual Issues and PRs related to the Manual/Tutorials section of the documentation topic:animation labels Jun 9, 2025
@AThousandShips AThousandShips added this to the 4.5 milestone Jun 9, 2025
@berarma berarma force-pushed the playing_videos_4.5 branch from 3abcd3b to aa7351f Compare June 17, 2025 14:11
@berarma
Copy link
Contributor Author

berarma commented Jun 17, 2025

After testing Shutter Encoder myself I've decided against including it in the docs because it uses the worst possible encoding parameters and offers no way to change them.

Copy link
Contributor

@skyace65 skyace65 left a comment

Choose a reason for hiding this comment

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

Looks good! Only one minor change needs to be made.

Comment on lines +203 to +205
Current official FFmpeg releases have some bugs in their Ogg/Theora
multiplexer. It's highly recommended to use one of the latest static daily
builds, or build from their master branch where they are already fixed.
Copy link
Contributor

@skyace65 skyace65 Jun 23, 2025

Choose a reason for hiding this comment

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

Could you please specify an exact version here? Something like "at the time of writing daily builds are fixed, expect that anything newer than 7.1.1 should be fine." But it doesn't have to be worded like that exactly. Just something so that in the future people aren't wondering what exact version fixed the issue.

Copy link
Contributor Author

@berarma berarma Jun 23, 2025

Choose a reason for hiding this comment

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

I don't know. It could be fixed in a hypotetical 7.1.2 (less likely), in 7.2.0 (more likely?), or in 8.0 (most probably). I don't know how the release process works. I avoided mentioning versions because of this.

The FFmpeg project page recommends that end users use the git source anyway because it will have the latest fixes. The releases are meant for repackagers and integrators. Maybe we should do the same.

Copy link
Contributor Author

@berarma berarma Jun 23, 2025

Choose a reason for hiding this comment

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

This is from ffmpeg.org:

Approximately every 6 months the FFmpeg project makes a new major release. Between major releases point releases will appear that add important bug fixes but no new features. Note that these releases are intended for distributors and system integrators. Users that wish to compile from source themselves are strongly encouraged to consider using the development branch (see above), this is the only version on which FFmpeg developers actively work. The release branches only cherry pick selected changes from the development branch, which therefore receives much more and much faster bug fixes such as additional features and security patches.

Maybe saying something along the lines of "it's recommended to build from the sources using the development branch or use the daily builds to get the latest features and fixes" as a permanent notice. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about this, do we have a link to the (presumably closed) theora issue in their repository? Or whatever PR fixed it? If we do I'd like it to be linked in a doc comment. That way in 6 months or whenever we can check to see "is this in a release yet?" I just don't like idea of an indefinite "build from source" warning with no easy way to see if the issue fix has been included in a release yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had initially linked to the closed issues in their Trac page, but I was suggested that it was too much, and I agreed. Also, it doesn't seem to provide information on which release fixes them.

Notice that although FFmpeg suggests building from source, the download page has links to daily builds under "More downloading options". So it isn't mandatory to build from source.

These are the issues:

https://trac.ffmpeg.org/ticket/11451
https://trac.ffmpeg.org/ticket/11454

They don't work with PRs, they send patches to a mailing list instead.

These are the commits fixing those issues:

FFmpeg/FFmpeg@22aa71d
FFmpeg/FFmpeg@84d85e7
FFmpeg/FFmpeg@6e26f57

I don't think there's an easy way to know that they're included in a release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation topic:animation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants