Added setting rotation metadata

[jebw]

Added code for setting the rotate metadata

There is also a new method which will set the rotate filter and adjust the existing metadata accordingly.

[attenzione]

i'm confused about method naming, to keep similar with filter_rotate i think better would be metadata_rotate instead of set_rotate_metadata, and filter_rotate_and_update_metadata is unnecessary, this should be inside filter_rotate

[owahab]

Lacks testing. Please fix.

[jebw]

Method naming - yep can change that.

The reason why there's a second method for the combined action is because there may be scenarios where you want to handle the two actions independently. Paperclip-transcoder is one such scenario, it will already know the rotate metadata, so can call the filter_rotate and metadata_rotate directly supplying appropriate values without needing to spawn another copy of ffprobe to load the meta data again just for this action.

The other two alternatives are

a) accepting duplicate calls to ffprobe which seems inefficient, or
b) adding some caching into ruby-av.

That second option is what I'd originally intended to do but the current api (Av#identify) takes the source file as a parameter so you'd end up needing to build a hash of all the queried paths, and need to add ways to invalidate the cache, etc

Hence the simpler approach of assuming the higher level app/library knows best. If it wants a simple single command then it can call filter_rotate_and_update_metadata (or whatever it gets called), and if the app wants more efficiency/control it can call the filter_rotate/metadata_rotate directly.

[jebw]

Re testing - the lack of tests is because there weren't any tests for the existing rotation support.

What would you want testing? Presumably you want tests for the conversion command line being generated?

[owahab]

Yep, I think this will suffice.

[jebw]

I've added a couple of tests for the methods I added I've also renamed the combined method to filter_metadata_rotate - I could do with some advice for the name since I'm not sure what the conventions are.

[owahab]

Can we make this clockwise instead of counterclockwise?

[jebw]

I've been doing some experiments with this and whilst I can see why the suggestion is to set the rotation information clockwise, this doesn't actually have the expected effect. I wonder if the rotation metadata is actually intended to convey how to play the video instead of conveying how it was rotated.

If I take a landscape video with no rotate metadata, then rotate it 90 degrees clockwise, and also set the metadata to 90 to show its been rotated 90 degrees - quicktime, vlc, ffplay all show the video upside down - ie 90 degrees rotation + another 90 for (the wrong) correction

Conversely if I take that video and set the metadata to 270, it plays correctly in Quicktime, VLC, ffplay?

[owahab]

I guess we won't set the metadata as long as players will rotate the video.

[jebw]

The desktop video players need the metadata in order to decide how much to rotate by. Conversely web based players ignore the metadata (I believe).

I think the rotate metadata describes the 'device (phone) rotation' which is the opposite of the video contents rotation.

[owahab]

Understood. What about the CW rotation (as opposed to the current CCW) implementation?

[owahab]

AFAIU, the way rotation works right now is by rotating minus given degrees (CCW), no?

[jebw]

I've not changed the rotation method (besides fixing the input/output params issue).

My understanding is that filter_rotate rotates clockwise.

Metadata_rotate sets the metadata to whatever you tell it.

Filter_metadata_rotate rotates the image by the desired amount, then corrects the meta data. If the image is rotated 90 CW, then the metadata should show the recording device as having been rotate 90 CCW, ie 270

[owahab]

I got this all wrong. Merging right now.

[jebw]

AFAICT the metadata shows device rotation not image rotation

[jebw]

Thx

[owahab]

Thank you Jeb for the test cases. Once we agree about the rotation, I'll merge this.