On Sat, Oct 8, 2016 at 1:39 PM, Michael Niedermayer <mich...@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 >> <mich...@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.giov...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel