[FFmpeg-devel] [PATCH] mov: Evaluate the movie display matrix

[Vittorio Giovara]

On Sat, Oct 8, 2016 at 1:39 PM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Fri, Oct 07, 2016 at 10:38:22PM -0400, Vittorio Giovara wrote:
>> On Fri, Oct 7, 2016 at 8:38 PM, Michael Niedermayer
>> <michael at niedermayer.cc> wrote:
>> > On Fri, Oct 07, 2016 at 03:31:46PM -0400, Vittorio Giovara wrote:
>> >> This matrix needs to be applied after all others have (currently only
>> >> display matrix from trak), but cannot be handled in movie box, since
>> >> streams are not allocated yet.
>> >>
>> >> So store it in main context and if not identity, apply it when appropriate,
>> >> handling the case when trak display matrix is identity and when it is not.
>> >>
>> >> Signed-off-by: Vittorio Giovara <vittorio.giovara at gmail.com>
>> >> ---
>> >
>> >> +        } else { // otherwise multiply the two and store the result
>> >> +            int64_t val = 0;
>> >> +            for (i = 0; i < 3; i++) {
>> >> +                for (j = 0; j < 3; j++) {
>> >> +                    int sh = j == 2 ? 30 : 16;
>> >> +                    for (e = 0; e < 3; e++) {
>> >> +                        val += CONV_FP2INT(display_matrix[i][e], sh) *
>> >> +                               CONV_FP2INT(c->movie_display_matrix[e][j], sh);
>> >
>> > This does not work (you are dividing the 32bit numbers down to 2 bit)
>>
>> I don't understand this comment, are you referring to the fact that
>> the first two columns of the display matrix are 16.16, while the third
>> column is 2:30?
>
> you have 32bit integers and sh can be 30, in that case you throw away
> 30 of 32 bits before doing anything with the number

yes but that value will be between 0 and 0.999999, which can be safely ignored.

>> > also its not tested by the fate testcase
>> > i can just replace it by 0 and fate-mov-movie-display-matrix still
>> > passes
>>
>> Yes, the sample I shared only has the movie display matrix, not
>> trak+movie. I can create another sample if you insist, but a fate test
>> would test little more than the matrix multiplication loops.
>
> yes

ok i'll simplify the code altogether

>> > the macros also lack some () protection giving probably unintended
>> > results
>>
>> Can you tell me how many more () would they need?
>
> #define CONV_FP2INT(x, sh) ((int64_t) (x)) / (1 << sh)
>
> is missing at least 2 pairs
>
> #define CONV_FP2INT(x, sh) (((int64_t) (x)) / (1 << (sh)))

thanks, applied

> also the rounding is not optimal it should likely be rounding to
> nearest
> the division can be replaced by a shift while correcting the rounding

the rounding is fine, as i said the error is below 1 which is
acceptable for transform operations
also i really doubt using int64 is useful here, given that the numbers
are immediately converted to double when computing disp_transform and
hypot. I actually fear that using int64 will increase the error so I'm
tempted to drop it.
-- 
Vittorio