Missing vertical/horizontal flip

[lotharkript]

since Oct 18, 2016, with https://github.com/FFMS/ffms2/commit/2ed27a5067aa2e8aedb809f19ac7941d770775dd the rotation is exposed, but does not include flip.

For flip, you are supposed to look at the determinant of the matrix: determinant = matrix[0,0] matrix[1,1] - matrix[0.1]matrix[1,0]

If the determinant is negative, we have a horizontal flip and we are suppose to apply a horizontal flip to the matrix from left before getting the rotation angle.

also, if the rotation is 180 and the determinant is negative, we should just apply an vertical flip and not a rotation of 180

Will it not be better to expose the whole matrix and let user to decide how to interpret it, instead of doing it in the code?

[myrsloik]

I didn't know that case existed. Do you have a sample file?

[lotharkript]

Yes I do.. what is the best way for me to share it with you?

[lotharkript]

Here is a file with a Horizontal flip. The file was auto generated. (I had to gzip it to upload it)

qrvideo6fps1s_hflip.mov.gz

[dwbuiten]

Will it not be better to expose the whole matrix and let user to decide how to interpret it, instead of doing it in the code?

Not sure I agree with that because it is MOV(/MP4) specific, and kind of goes against FFMS2's policy of a 'simpler' API than libavformat/libavcodec.

Anywhow...

As it stands, this is a rather ugly edge case (which MOV(/MP4) have loads of... not even sure why such stuff needs to exist) within libavformat, IMO. I'd like to hear from @kodabb, who added support to FFMS2 in the first place (and maybe lavf too?). It's possible this is better handled inside libavformat, but I have no strong opinions.

Thanks for the sample, I'll throw it at Vittorio on monday if he hasn't materialized with an opinion by then.

[kodawah]

Lavf outputs the whole matrix so if ffms2 were to design an api for hflip/vflip while not exposing the matrix, it would just need to call av_display_rotation_get and check for the transpose values. If i remember correctly the horizontal/vertical flip in a display matrix are simply set in matrix[6] and matrix[7] respectively.

[myrsloik]

Here's another crazy idea. How about we just do all the rotation and flipping before returning a frame? That'd also solve things. Unless there's some really crazy case....

[dwbuiten]

I strongly object to FFMS2 doing auto-rotation unless specifically enabled. Don't mess with my buffers...

[kodawah]

I didn't know which one to pick

[kodawah]

BTW rotating frames automatically (but not flipping) is what the ffmpeg CLI does, but the API returns them raw, and provides means to notify the user that they should be rotated (or flipped)

The rationale for exposing the display matrix is that it can perform so many more operations that are hard to express with a series of metadata values. I agree this conflicts with the "simple" ffms2 api, so maybe adding fields and values on a use-case basis could work too.

[kodawah]

looking at the sample, the display matrix is the classic 180º rotation, there is nothing about flipping

-1 0 0
0 1 0
0 0 1

i'm not sure how quicktime displays it differently, probably an unknown/unsupported box?

[lotharkript]

This is what we found about rotation and flip (manipulating the matrix and using quicktime to compare result) pseudo code here:

det = matrix[0]*maxtrix[4] - matrix[1]*matrix[3]
if det < 0 {
  // Need a horizontal flip
  // And need to apply a horizontal flip to the matrix 
  // as we can have rotation and horizontal flip.
}
// av_display_rotation gives angle in range [-180, 180] in counter clock wise direction
// Map the angle to be [0,360]

if the angle = 180 {
  if  det < 0 {
    // this is a verticaflip. No need of rotation
  } else {
    // rotation of 180
  }
}

So, a original matrix of

-1 0 0
 0 1 0
 0 0 1

Will give us a horizontal flip, not a rotation by 180

[dwbuiten]

I think @lotharkript is correct. Wouldn't this be a 180 degree rotation?

-1  0  0
 0 -1  0
 0  0  1

[tgoyne]

I would expect ffms2 to automatically perform rotation and flipping if a specific output format is requested, and otherwise just report the applicable information.

[kodawah]

I think @lotharkript is correct. Wouldn't this be a 180 degree rotation?

yes that should be right, i was wrong

the code in ffmpeg performing rotation is a bit different (using euclidean distance) than the pseudo code, maybe it could be changed to report 0 and introduce a separate function to report flipping?

    double rotation, scale[2];

    scale[0] = hypot(CONV_FP(matrix[0]), CONV_FP(matrix[3]));
    scale[1] = hypot(CONV_FP(matrix[1]), CONV_FP(matrix[4]));

    if (scale[0] == 0.0 || scale[1] == 0.0)
        return NAN;

    rotation = atan2(CONV_FP(matrix[1]) / scale[1],
                     CONV_FP(matrix[0]) / scale[0]) * 180 / M_PI;

    return -rotation;

[kodawah]

@lotharkript do you happen to have a sample or pseudocode for a vflip case?

[dwbuiten]

I would expect ffms2 to automatically perform rotation and flipping if a specific output format is requested, and otherwise just report the applicable information.

What sort of output format do you mean? I would expect it to be a field as now, unless I explicitly ask for rotated output.

@lotharkript do you happen to have a sample or pseudocode for a vflip case?

Here's one I made with QuickTime Pro: vflip.mov.gz

Transformation matrix is as you would expect.

[kodawah]

Transformation matrix is as you would expect.

1 0 0
0 -1 0
0 480 1

let's exclude transpose y, the determinant should be negative and we could be able to determine the flipping instead of rotation

[isasi]

@kodabb We don't want to just return a rotation value of zero, if there is a flip. What if the rotation is 45 degree and the determinant is negative.

[kodawah]

so after thinking about this for a bit, and speaking to other community members, I came to the conclusion that av_display_rotation_get is too limited for this usecase, and we need to introduce a new API. If i understand the matrix operation correctly, we'd need to define 8 possible operations and report the well known display orientations as follows

enum AVDisplayOrientation {
    AV_IDENTITY,
    AV_ROTATION_90,
    AV_ROTATION_180,
    AV_ROTATION_270,
    AV_FLIP_HORIZONTAL,
    AV_FLIP_VERTICAL,
    AV_TRANSPOSE,
    AV_ANTITRANSPOSE,
};

enum AVDisplayOrientation av_display_orientation_set(int32_t matrix[9])
{
    enum AVDisplayOrientation orientation = AV_IDENTITY;
    int32_t det = matrix[0] * maxtrix[4] - matrix[1] * matrix[3];

    if (matrix[3] == -65536 && matrix[1] == 65536)
        orientation = AV_ROTATION_90;
    else if (matrix[0] == -65536 && matrix[4] == -65536)
        orientation = AV_ROTATION_180;
    else if (matrix[3] == 65536 && matrix[1] == -65536)
        orientation = AV_ROTATION_270;

    if (det < 0) {
        if (orientation == AV_ROTATION_180)
            orientation = AV_FLIP_HORIZONTAL;
        else if (orientation == AV_ROTATION_90 || orientation == AV_ROTATION_270)
            orientation = AV_FLIP_VERTICAL;
    }

    return orientation;
}

What do people think of this idea? This would not deprecate old API as it would serve a different purpose. I am not too sure about how to get the transpose operation, if you have any clue for it, I'd be happy to hear it.

[lotharkript]

Here are some samples with flip and rotation.

qrvideo6fps1s_hflip_90_ccw.mov.gz qrvideo6fps1s_hflip_90_cw.mov.gz qrvideo6fps1s_vflip.mov.gz

Remember, you can have rotation + flip at the same time. You may have to signal (AV_ROTATION_90 | AV_FLIP_HORIZONTAL)

[isasi]

What about non-90 degree rotations like 45 degrees . I feel like the API shouldn't restrict the rotation angle to be only a multiple of 90

[kodawah]

@lotharkript thanks for the samples, and good point about rotation + flip.

@isasi hm we could have a AV_ROTATION_CUSTOM and document to use av_display_rotation_get to get the angle in degrees in that case

[lotharkript]

@kodabb Don't you need to apply a Horizontal flip to the matrix before you compute the rotation? (if there is a Horizontal flip)

[kodawah]

@lotharkript yes, I believe so, this is what we do for example in h264 (h264_slice.c: 1257) when the relevant SEI is found and we need to set the display matrix

            av_display_rotation_set((int32_t *)rotation->data, angle);
            av_display_matrix_flip((int32_t *)rotation->data, o->hflip, o->vflip);

But for getting the display matrix, how would you be able to know the direction of the flip operation beforehand? Right now, in the code I posted,det is computed so to know whether there is a flip or not, but then I need to check which direction the flip is, and I do so by checking the rotation itself.

Should the flip operation be determined first (getting rotation and direction as in the PoC), and then retrieve the rotation from the flipped matrix?

[isasi]

@kodabb

Yes, I think so . the rotation that should be applied will depend on the flip direction being applied and also whether the flip is being applied before or after the rotation. For example Horizontal Flip ~ Vertical Flip and then 180 degree counter clockwise rotation.

It doesn't matter what flip direction we return (Hflip or Vflip ) as long as we establish i) which should be applied first - flip or rotation ii) rotation should be calculated keeping in consideration the flip being applied.

[kodawah]

how's this looking?

    enum AVDisplayOrientation orientation = 0, flip_check = 0;
    int32_t det = matrix[0] * maxtrix[4] - matrix[1] * matrix[3];
    int32_t matrix[9];

    memcpy(matrix, matrix_src, sizeof(*matrix_src) * 9);

    if (matrix[3] == -65536 && matrix[1] == 65536)
        flip_check = AV_ROTATION_90;
    else if (matrix[0] == -65536 && matrix[4] == -65536)
        flip_check = AV_ROTATION_180;
    else if (matrix[3] == 65536 && matrix[1] == -65536)
        flip_check = AV_ROTATION_270;
    else if (matrix[0] == 65536 && matrix[4] == 65536)
        flip_check = AV_IDENTITY;

    if (det < 0) {
        if (flip_check == AV_ROTATION_180 || flip_check == AV_IDENTITY) {
            orientation = AV_FLIP_HORIZONTAL;
            av_display_matrix_flip(matrix, 1, 0);
        } else if (flip_check == AV_ROTATION_90 || flip_check == AV_ROTATION_270) {
            orientation = AV_FLIP_VERTICAL;
            av_display_matrix_flip(matrix, 0, 1);
        }
    }

    if (matrix[3] == -65536 && matrix[1] == 65536)
        orientation |= AV_ROTATION_90;
    else if (matrix[0] == -65536 && matrix[4] == -65536)
        orientation |= AV_ROTATION_180;
    else if (matrix[3] == 65536 && matrix[1] == -65536)
        orientation |= AV_ROTATION_270;
    else if (matrix[0] == 65536 && matrix[4] == 65536)
        orientation |= AV_IDENTITY;
    else
        orientation |= AV_ROTATION_CUSTOM;

    return orientation;

[isasi]

>     if (matrix[3] == -65536 && matrix[1] == 65536)
>         flip_check = AV_ROTATION_90;
>     else if (matrix[0] == -65536 && matrix[4] == -65536)
>         flip_check = AV_ROTATION_180;
>     else if (matrix[3] == 65536 && matrix[1] == -65536)
>         flip_check = AV_ROTATION_270;
>     else if (matrix[0] == 65536 && matrix[4] == 65536)
>         flip_check = AV_IDENTITY;

Shouldn't this be

    if (matrix[3] == **+**65536 && matrix[1] == 65536)
        flip_check = AV_ROTATION_90;
    else if (matrix[0] == -65536 && matrix[4] == **+**65536)
        flip_check = AV_ROTATION_180;
    else if (matrix[3] == **-**65536 && matrix[1] == -65536)
        flip_check = AV_ROTATION_270;
    else if (matrix[0] == 65536 && matrix[4] == **-**65536)
        flip_check = AV_IDENTITY;

because if there is flip, then signs need to be different to make det negative.

Also, I think the easier way would be to first flip the matrix assuming HORIZONTAL flip , right after we find det < 0 . and then subsequently if we find that rotation is AV_ROTATION_180, we can just apply vertical flip instead of horizontal flip + 180 rotation.

   enum AVDisplayOrientation orientation = 0, flip_check = 0;
    int32_t det = matrix[0] * maxtrix[4] - matrix[1] * matrix[3];
    if (det < 0) { 
            orientation = AV_FLIP_HORIZONTAL;
            av_display_matrix_flip(matrix, 1, 0);
     } 

    if (matrix[3] == -65536 && matrix[1] == 65536)
        orientation |= AV_ROTATION_90;
    else if (matrix[0] == -65536 && matrix[4] == -65536) { 
        if (det < 0) { 
            orientation = AV_FLIP_VERTICAL;
        } else {
            orientation |= AV_ROTATION_180;
        }
    }
    else if (matrix[3] == 65536 && matrix[1] == -65536)
        orientation |= AV_ROTATION_270;
    else if (matrix[0] == 65536 && matrix[4] == 65536)
        orientation |= AV_IDENTITY;
    else
        orientation |= AV_ROTATION_CUSTOM;

    return orientation;

[kodawah]

@isasi thanks for your suggestions

regarding the first part it depends if we want to break compatibility with the past or not: using your set of values the rotation operations will be counterclockwise and compatible with the current av_display_rotation_get(), using mine rotations will be clockwise. I personally think that the counterclockwise rotation were always a mistake, so i wanted to "fix" the api by returning clockwise operations. Do you think that's a bad idea?

regarding the second part, yes the code indeed can be simplified, I've folded it in my changes locally

[kodawah]

FYI I sent a patch to ffmpeg/libav-devel we can continue the discussion there

[kodawah]

so almost a year later I picked this up again... please let me know your thoughts

https://github.com/FFMS/ffms2/pull/342