Only get rotation from metadata if 0 keyframes #696
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #696 +/- ##
===========================================
+ Coverage 50.41% 51.12% +0.71%
===========================================
Files 155 155
Lines 13315 13648 +333
===========================================
+ Hits 6713 6978 +265
- Misses 6602 6670 +68
Continue to review full report at Codecov.
|
| @@ -122,12 +122,9 @@ void Clip::init_reader_settings() { | |||
| // Init reader's rotation (if any) | |||
| void Clip::init_reader_rotation() { | |||
| // Only init rotation from reader when needed | |||
| if (rotation.GetCount() > 1) | |||
| if (rotation.GetCount() > 0) | |||
There was a problem hiding this comment.
I'm not sure this is right, won't rotation.GetCount() always be > 0? the Keyframe objects are initialized with a default rotation of 0.0. (Which is kind of the issue, the original change was intended to correct that initial value unless the user modified it. Problem is, there's no way to be sure of that.)
i really think the way this needs to work is, any metadata rotation needs to be read and applied as a separate adjustment from rotation. I've been calling it "intrinsic rotation", which is the rotation baked into the video. We'd want to store that as a separate property in the ReaderInfo, completely independent of any rotation keyframes. If info.rotation is present, that'd be applied to each individual Frame before returning it to the caller. Especially because, if it's a 90° or 270° rotation, not only should the video be rotated, but the width and height attributes need to be transposed accordingly. A 1080×1920 portrait video with an intrinsic rotation of 90° should produce Frame objects containing 1920×1080 images, which no amount of Keyframe shenanigans can achieve..
That will necessarily be a breaking change incompatible with older projects, but I really think it's the only way to solve this properly. I'd be happy to be proven wrong, but I haven't really seen any other fix yet, and the band-aids we've applied so far all seem less than satisfactory.
There was a problem hiding this comment.
Yeah, this a tough one. I've abandoned a few different attempts to fix this. But, I believe we do not initialize rotation until we check the reader's metadata, and thus, it "should" be 0 Points during the construction during this logic. However, I have not personally tested this yet. If we really do have 1 Point at all times, regardless, than yes, we might need to implement this logic differently.
Technically, we do have a persistent place to check for the intrinsic rotation, since it's always cached in the reader metadata. However, it would be very cool for the FFmpegReader to rotate the pixels sooner in the stack, so everything else just "works".
But again, I would settle for an improved quick fix, if we can verify that rotation is only initialized 1 time, only in the constructor, and only based on metadata. And any adjustments afterwords would win.
There was a problem hiding this comment.
From what I've seen, I'm pretty sure the clip does have 0 rotation keyframes the first time this gets called.
Just understood the later part of this method is copying changes throughout the video. I'll take a video that changes rotation and check this change.
(Just realized this comment was 'pending' over the weekend.)
There was a problem hiding this comment.
By 'Just Works', do you mean show 0 degrees, regardless of the intrinsic rotation?
There was a problem hiding this comment.
From what I've seen, I'm pretty sure the clip does have 0 rotation keyframes the first time this gets called.
Oh! Cool, that's handy. Good deal.
By 'Just Works', do you mean show 0 degrees, regardless of the intrinsic rotation?
Basically, yeah, it would be what I described initially: A clip with an intrinsic rotation of 90° (for example) has its dimensions transposed to match the rotated shape, and comes out of the reader already rotated. So the Keyframe rotation remains 0 unless the user applies some. That'd be the "holy grail". (Though, like I said, it's also unavoidably a breaking change with older projects. Though we could theoretically use the libopenshot version in the project file data to mitigate that, if we made the change on a version bump and left code in to apply the rotation only conditionally.)
There was a problem hiding this comment.
(Most rotated video is done by things like phone cameras... the user doesn't know that it's rotated, doesn't care that it's rotated, and shouldn't have to deal with the fact that it's rotated. To them it should just be a video in the orientation they shot it in, whatever that may be.)
...Especially since cellphone video rotation is pretty counterintuitive, for the lay person — typically the unrotated orientation is landscape, while portrait video is a 90° rotation. So, the video the user thinks of as unrotated (portrait) is actually rotated, and when they shoot with their phone rotated the video comes out unrotated.
There was a problem hiding this comment.
Technically, we do have a persistent place to check for the
intrinsic rotation, since it's always cached in the reader metadata.
That's true, but adding it to the metadata explicitly would give us another way to detect whether a clip already has its rotation pre-applied. No data member, no rotation. (We could also make it an integer or enum value, instead of having to parse it out of a string.)
Changing rotation data assumed that a video file's starting rotation is 0 degrees.
This change allows users to set a 0 degree keyframe when there is only one rotation keyframe.