Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is right, won't
rotation.GetCount()
always be> 0
? theKeyframe
objects are initialized with a default rotation of0.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. Ifinfo.rotation
is present, that'd be applied to each individualFrame
before returning it to the caller. Especially because, if it's a 90° or 270° rotation, not only should the video be rotated, but thewidth
andheight
attributes need to be transposed accordingly. A1080×1920
portrait video with an intrinsic rotation of 90° should produceFrame
objects containing1920×1080
images, which no amount ofKeyframe
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By 'Just Works', do you mean show 0 degrees, regardless of the intrinsic rotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! Cool, that's handy. Good deal.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonoomph
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.)