search

FFMS / ffms2

An FFmpeg based source library and Avisynth/VapourSynth plugin for easy frame accurate access
Other
582 stars 105 forks source link

Expose flip operations from the display matrix #342

Closed kodawah closed 5 years ago

kodawah commented 5 years ago

Fix #317.

kodawah commented 5 years ago

some notes:

lotharkript commented 5 years ago

can we not have the full matrix instead of all those fields?

kodawah commented 5 years ago

@lotharkript I thought that there was consensus last year to avoid exposing it and keep the ffms2 api as simple as possible. Also Flip is the last field to be needed, given that all the other operations from the matrix (rotation, scale, shear) are either supported differently or do not apply to a video surface

dwbuiten commented 5 years ago
  • the VP.Rotation values do not change if the video is not v/h flipped
  • they do change if it is, but the code was doing the wrong thing anyway -- applications need to check the VP.Flip value

It's not clear from the API or documentation what will change in their values and/or what order to apply things, to me.

is it worth to introduce some enums for the flip values instead of positive/negative states?

IMO, no.

should i add unit tests using the samples from the bug?

They can probably be covered pretty easily, possibly within the existing tests (?)

I thought that there was consensus last year to avoid exposing it and keep the ffms2 api as simple as possible. Also Flip is the last field to be needed, given that all the other operations from the matrix (rotation, scale, shear) are either supported differently or do not apply to a video surface.

Are we sure we don't need to care about things like shear/scale/etc? As in, What Would QuickTime Do? (WWQTD).

Are there any other benefits to exposing the matrix directly? That could get pretty ugly for non-mp4/mov formats, as you'd have to expose that info differently, or build of a matrix to export.

kodawah commented 5 years ago

It's not clear from the API or documentation what will change in their values and/or what order to apply things, to me.

The change is that we flip the matrix before we compute the rotation angle. So if you had hflipped video, before you had a rotation of 180 while now you have rotation = 0 and flip = 1. It's technically an abi break but it only applies to videos that were previously unsupported and which we we were reporting false information of.

They can probably be covered pretty easily, possibly within the existing tests (?)

yeah i can just copy the matrix values from the test files, but the sample ones are pretty small anyway

Are we sure we don't need to care about things like shear/scale/etc? As in, What Would QuickTime Do? (WWQTD).

Are there any other benefits to exposing the matrix directly? That could get pretty ugly for non-mp4/mov formats, as you'd have to expose that info differently, or build of a matrix to export.

I'll let @lotharkript answer this :)

kodawah commented 5 years ago

updated the comment, waiting on instructions on what to do with tests and the matrix :)

kodawah commented 5 years ago

I slightly modified the logic for how rotation is handled -- from the commit log

    It should be noted that previously the rotation value was used as is
    and it was modified to be always positive by adding 360. However since
    flip operations are exposed now, it is necessary to apply some more
    logic and internally convert the rotation angle.

    For non-flipped videos this should be a transparent change.
dwbuiten commented 5 years ago

I'll wait for a comment from @lotharkript on the whole matrix thing, unless he has no strong feelings on it.

dwbuiten commented 5 years ago

Having not heard from him, I'll merge once a unit test is added.