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

[Michael Niedermayer]

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


> 
> > 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


> 
> > 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)))

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


> 
> From the subsequent email
> 
> > to explain a bit more how to do it with int64 instead of double floats
> > int32 can be multiplied together to form int64 with a new fixed point
> > without rounding or shifts
> > then added up and then at the end shifted down with rounding to get
> > back to the fixed point needed for the final destination
> 
> The problem is that the display matrix fields have different mantissa
> length, the first two columns are 16.16, the last one is 2:30. I think
> that method would work if all elements were the same length no?

iam not sure the original double code is correct, but it used the same
shift for each output, just changing between outputs.

if that was correct it should work fine, if it was not correct then
some extra shift is needed after the multiplication and before the
addition to get things to match


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161008/bdf2bd32/attachment.sig>