search

Rafostar / clapper

Level up your video experience with a modern and user-friendly media player.
https://rafostar.github.io/clapper/
GNU General Public License v3.0
714 stars 34 forks source link

Allow detecting and setting media rotation #349

Closed g7 closed 1 year ago

g7 commented 1 year ago

Fixes #310

Rotation is done through GskTransforms directly on the Snapshot.

The default behaviour is to autodetect the media rotation if available, but frontends can force it by setting the rotation property.

I've also exposed this to the actual app, but I'm unsure if that's wanted :sweat_smile:

This is how it looks now (playing the video from the aforementioned bug report):

Video del 2023-04-01 15-36-57.webm

Thanks for your work on clapper!

Rafostar commented 1 year ago

Thanks for working on this 👍

Rotation is done through GskTransforms directly on the Snapshot.

Yes, this is the correct behaviour I had in mind for this feature.

Rafostar commented 1 year ago

I've also exposed this to the actual app, but I'm unsure if that's wanted

As for adding this to the GUI, I would probably prefer to not clutter it too much with buttons for rarely needed/used option. The "auto" behaviour should already fix the issue you linked. For the rest of cases where it should not be rotated, just user wishes to do so for some reason, I would probably simply put a keyboard shortcuts for +/- 90 degrees changes. Does not have to be done as part of this MR.

If this is alright with you, I would keep it for now for testing purposes of this MR and remove before merging.

g7 commented 1 year ago

Thanks for the review.

If this is alright with you, I would keep it for now for testing purposes of this MR and remove before merging.

Sure.

Updated the pull request with changes addressing the raised issues, including the implementation of the missing methods. I've added the commits on top rather than squashing directly to ease the review.

The reason why I added a custom enum was to avoid cluttering the UI side with too many options, so I kept only the ones that made sense to me. But obviously since we're going to get rid of that part altogether that's not a worry anymore.

g7 commented 1 year ago

v3:

use a switch in gst_gtk_get_width_height_for_rotation, uniformise the indentation in gst_clapper_paintable_snapshot_internal with the rest of the code

Rafostar commented 1 year ago

Seems OK to me. Also tried if mouse navigation coordinates are correct with this and they seem to be. Good job :+1:

In your free time please clean commits of this, removing changes to the UI as we discussed earlier and we will be able to get this in.

g7 commented 1 year ago

Thanks!

v4: