[FFmpeg-devel] avutil/display: fix inverted doc

---
 libavutil/display.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Zhao Zhili:
> ---
>  libavutil/display.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavutil/display.h b/libavutil/display.h
> index 515adad795..d87bf68425 100644
> --- a/libavutil/display.h
> +++ b/libavutil/display.h
> @@ -88,7 +88,7 @@
>  double av_display_rotation_get(const int32_t matrix[9]);
>  
>  /**
> - * Initialize a transformation matrix describing a pure counterclockwise
> + * Initialize a transformation matrix describing a pure clockwise
>   * rotation by the specified angle (in degrees).
>   *
>   * @param matrix an allocated transformation matrix (will be fully overwritten
> 

If you look at the documentation, you will notice that the matrix
returned from the function is supposed to be multiplied from the right
to the coordinate (row) vector, not from the left to the coordinate
(column) vector as is standard in linear algebra. This is tantamount to
transposing the matrix which for rotation matrices means negating the
degree. So this doxy is correct.
Notice that last time I looked at this, I came to the conclusion that
the whole H.2645 SEI parsing code uses it wrongly.

- Andreas

Andreas Rheinhardt:
> Zhao Zhili:
>> ---
>>  libavutil/display.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavutil/display.h b/libavutil/display.h
>> index 515adad795..d87bf68425 100644
>> --- a/libavutil/display.h
>> +++ b/libavutil/display.h
>> @@ -88,7 +88,7 @@
>>  double av_display_rotation_get(const int32_t matrix[9]);
>>  
>>  /**
>> - * Initialize a transformation matrix describing a pure counterclockwise
>> + * Initialize a transformation matrix describing a pure clockwise
>>   * rotation by the specified angle (in degrees).
>>   *
>>   * @param matrix an allocated transformation matrix (will be fully overwritten
>>
> 
> If you look at the documentation, you will notice that the matrix
> returned from the function is supposed to be multiplied from the right
> to the coordinate (row) vector, not from the left to the coordinate
> (column) vector as is standard in linear algebra. This is tantamount to
> transposing the matrix which for rotation matrices means negating the
> degree. So this doxy is correct.
> Notice that last time I looked at this, I came to the conclusion that
> the whole H.2645 SEI parsing code uses it wrongly.
> 

Wait, there is another difference to ordinary linear algebra: The y axis
is directed downwards. Maybe you are right. Let me think this through...

- Andreas

> On Dec 12, 2021, at 7:19 PM, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> 
> Andreas Rheinhardt:
>> Zhao Zhili:
>>> ---
>>> libavutil/display.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/libavutil/display.h b/libavutil/display.h
>>> index 515adad795..d87bf68425 100644
>>> --- a/libavutil/display.h
>>> +++ b/libavutil/display.h
>>> @@ -88,7 +88,7 @@
>>> double av_display_rotation_get(const int32_t matrix[9]);
>>> 
>>> /**
>>> - * Initialize a transformation matrix describing a pure counterclockwise
>>> + * Initialize a transformation matrix describing a pure clockwise
>>>  * rotation by the specified angle (in degrees).
>>>  *
>>>  * @param matrix an allocated transformation matrix (will be fully overwritten
>>> 
>> 
>> If you look at the documentation, you will notice that the matrix
>> returned from the function is supposed to be multiplied from the right
>> to the coordinate (row) vector, not from the left to the coordinate
>> (column) vector as is standard in linear algebra. This is tantamount to
>> transposing the matrix which for rotation matrices means negating the
>> degree. So this doxy is correct.
>> Notice that last time I looked at this, I came to the conclusion that
>> the whole H.2645 SEI parsing code uses it wrongly.


Yes, there are some issues related to the SEI part:

./ffmpeg -i /tmp/test.h264 -c copy -bsf h264_metadata=rotate=90:display_orientation=insert  /tmp/90.h264 -y


ffmpeg -i /tmp/90.h264 -vf showinfo -f null - 2>&1 |grep displaymatrix
[Parsed_showinfo_0 @ 0x7f8980a19d40]   side data - displaymatrix: rotation of -90.00 degrees
[Parsed_showinfo_0 @ 0x7f8980a19d40]   side data - displaymatrix: rotation of -90.00 degrees

>> 
> 
> Wait, there is another difference to ordinary linear algebra: The y axis
> is directed downwards. Maybe you are right. Let me think this through...
> 
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Zhao Zhili:
> ---
>  libavutil/display.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavutil/display.h b/libavutil/display.h
> index 515adad795..d87bf68425 100644
> --- a/libavutil/display.h
> +++ b/libavutil/display.h
> @@ -88,7 +88,7 @@
>  double av_display_rotation_get(const int32_t matrix[9]);
>  
>  /**
> - * Initialize a transformation matrix describing a pure counterclockwise
> + * Initialize a transformation matrix describing a pure clockwise
>   * rotation by the specified angle (in degrees).
>   *
>   * @param matrix an allocated transformation matrix (will be fully overwritten
> 

a) This change brings the documentation in line with actual behaviour.
b) This bug has been introduced in
e4fe535d12f4f30df2dd672e30304af112a5a827: The mov demuxer did not abide
by the documentation of the format of the display matrix side data (it
had an incorrect transposition). Instead of just fixing the mov demuxer
to abide by this format (which coincides with the native mov format!),
said commit also negated the degrees in av_display_rotation_get/set. In
case of av_display_rotation_get() this fixed a bug, in case of
av_display_rotation_set() this introduced a bug. No one seems to have
thought to actually test whether av_display_rotation_get() actually
returns the degree.
c) There is a test for this (libavutil/tests/display.c). It does not
perform the test just mentioned; instead it has the current behaviour
hardcoded in its ref file.
d) The rotate_override stuff in ffmpeg.c compensates for the weirdness
of av_display_rotation_set() by negating the degree. get_rotation in
fftools/cmdutils.c also negates the degree, but it seems to do so
deliberately in order to return a clockwise degree (both callers expect
it that way).
e) av_display_matrix_flip() is equivalent to multiplying the matrix with
a diagonal matrix (with diagonal entries +-1) from the right; given that
applying the displaymatrix corresponds to multiplying the coordinate row
vector with the matrix on the right, this means that flipping is applied
on top of the display matrix (i.e. after the transformation specified by
the earlier display matrix has been applied). This is in line with my
expectations of what it does given its documentation.
f) Yet it is not in line with how it is used by the H.2645 SEI parsing
code: According to the H.2645 specs, the flips should be applied first.
Flipping (regardless of axis) first, then rotating by phi is equivalent
to rotating by -phi first and then flipping around the same axis. This
implies that the code in libavcodec/hevcdec.c, libavcodec/h264_slice.c
and libavcodec/h264_metadata_bsf.c (when creating side data) leads to
correct results if there is exactly one flip; without a flip or two
flips, the sign of the degree is wrong. This is the reason for the issue
in your other mail [1].
g) The code to create an SEI from side data in h264_metadata_bsf.c is
incorrect.
h) I don't know whether the exif rotation code is correct; same for
android_camera.c.

- Andreas

[1]: http://ffmpeg.org/pipermail/ffmpeg-devel/2021-December/289488.html

Zhao Zhili:
> ---
>  libavutil/display.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavutil/display.h b/libavutil/display.h
> index 515adad795..d87bf68425 100644
> --- a/libavutil/display.h
> +++ b/libavutil/display.h
> @@ -88,7 +88,7 @@
>  double av_display_rotation_get(const int32_t matrix[9]);
>  
>  /**
> - * Initialize a transformation matrix describing a pure counterclockwise
> + * Initialize a transformation matrix describing a pure clockwise
>   * rotation by the specified angle (in degrees).
>   *
>   * @param matrix an allocated transformation matrix (will be fully overwritten
> 

Will apply this together with an APIchanges entry (and micro version
bump) that explains that the documentation has been modified to match
the actual behaviour.

- Andreas