Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Only get rotation from metadata if 0 keyframes #696

Merged
merged 2 commits into from
Jul 7, 2021
Merged

Only get rotation from metadata if 0 keyframes #696

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7d71cd
Only get rotation from metadata if 0 keyframes
JacksonRG Jul 1, 2021
92b6251
removed comment with number
JacksonRG Jul 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 1 addition & 4 deletions src/Clip.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

@ferdnyc ferdnyc Jul 2, 2021 •
edited

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? 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.

Copy link
Member

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.

Copy link
Collaborator Author

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.)

Copy link
Collaborator Author

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?

Copy link
Contributor

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.

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.)

Copy link
Contributor

@ferdnyc ferdnyc Jul 8, 2021 •
edited

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.

Copy link
Contributor

@ferdnyc ferdnyc Jul 8, 2021 •
edited

Choose a reason for hiding this comment

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

@jonoomph

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.)

// Do nothing if more than 1 rotation Point
return;
else if (rotation.GetCount() == 1 && rotation.GetValue(1) != 0.0)
// Do nothing if 1 Point, and it's not the default value
return;

// Init rotation
if (reader && reader->info.metadata.count("rotate") > 0) {
Expand Down