Em Wed, 27 Apr 2016 07:31:59 -0300 Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx> escreveu: > Hi Dan, > > Em Wed, 27 Apr 2016 11:09:28 +0300 > Dan Carpenter <dan.carpenter@xxxxxxxxxx> escreveu: > > > The > ARRAY_SIZE() should be >= ARRAY_SIZE(). > > I actually did this fix when I produced the patch, just I forgot to fold > it when merging. Anyway, this was fixed upstream by this patch: > https://git.linuxtv.org/media_tree.git/commit/?id=45c175c4ae9695d6d2f30a45ab7f3866cfac184b > > > Also this is a slightly > > unrelated cleanup but I replaced the magic numbers 30 and 25 with > > ARRAY_SIZE() - 1. > > I don't like magic numbers, but, in this very specific case, setting > frames per second (fps) var to 25 or 30 makes much more sense. The > rationale is that: > > The V4L2_STD_525_60 macro is for the Countries where the power line > uses 60Hz, and V4L2_STD_625_50 for the Countries where the power line > is 50Hz. > > The broadcast TV sends frames in half of this frequency, so, for > V4L2_STD_525_60, fps = 30, while, for V4L2_STD_625_50, fps = 25. > > So, in this very specific case, IMHO, it is better to see 25 or 30 there, > instead of ARRAY_SIZE(). > > That's said, I guess one improvement would be to get rid of those two > arrays and replacing them by a formula, like: > > i = (max_fps / 2 + 15 * fps) / max_fps; > if (i > 14) > i = 0; > > I'll propose such patch for evaluation. I did some tests to check how that array was determined, using the enclosed program. It seems that the table was generated using this formula: i2 = (adjust + 15 * fps) / max_fps; if (fps && !i2) i2 = 1; if (i2 > 14) i2 = 0; Where adjust is given by: adjust = 12; /* actually, any value between 11 and 14 */ Using it, I got these results: 60 Hz fps 0, i1=0, i2=0 fps 1, i1=1, i2=1 fps 2, i1=1, i2=1 fps 3, i1=1, i2=1 fps 4, i1=2, i2=2 fps 5, i1=2, i2=2 fps 6, i1=3, i2=3 fps 7, i1=3, i2=3 fps 8, i1=4, i2=4 fps 9, i1=4, i2=4 fps 10, i1=5, i2=5 fps 11, i1=5, i2=5 fps 12, i1=6, i2=6 fps 13, i1=6, i2=6 fps 14, i1=7, i2=7 fps 15, i1=7, i2=7 fps 16, i1=8, i2=8 fps 17, i1=8, i2=8 fps 18, i1=9, i2=9 fps 19, i1=9, i2=9 fps 20, i1=10, i2=10 fps 21, i1=10, i2=10 fps 22, i1=11, i2=11 fps 23, i1=11, i2=11 fps 24, i1=12, i2=12 fps 25, i1=12, i2=12 fps 26, i1=13, i2=13 fps 27, i1=13, i2=13 fps 28, i1=14, i2=14 NOK fps 29, i1=0, i2=14 fps 30, i1=0, i2=0 50 Hz fps 0, i1=0, i2=0 fps 1, i1=1, i2=1 fps 2, i1=1, i2=1 fps 3, i1=2, i2=2 NOK fps 4, i1=3, i2=2 fps 5, i1=3, i2=3 fps 6, i1=4, i2=4 fps 7, i1=4, i2=4 fps 8, i1=5, i2=5 fps 9, i1=5, i2=5 fps 10, i1=6, i2=6 fps 11, i1=7, i2=7 fps 12, i1=7, i2=7 fps 13, i1=8, i2=8 fps 14, i1=8, i2=8 fps 15, i1=9, i2=9 fps 16, i1=10, i2=10 fps 17, i1=10, i2=10 fps 18, i1=11, i2=11 fps 19, i1=11, i2=11 fps 20, i1=12, i2=12 fps 21, i1=13, i2=13 fps 22, i1=13, i2=13 fps 23, i1=14, i2=14 fps 24, i1=14, i2=14 fps 25, i1=0, i2=0 IMHO, the two values that are different at the table are wrong ;) I would also round to the closest number, with probably makes more sense, and fits well at the API requirements. The small program to test itis enclosed below. I'll send a patch getting rid of those tables. Regards, Mauro === #include <stdio.h> static const unsigned int std_625_50[26] = { 0, 1, 1, 2, 3, 3, 4, 4, 5, 5, 6, 7, 7, 8, 8, 9, 10, 10, 11, 11, 12, 13, 13, 14, 14, 0 }; static const unsigned int std_525_60[31] = { 0, 1, 1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 6, 6, 7, 7, 8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0 }; void calc_fps(unsigned int max_fps) { unsigned int i1, i2, fps, adjust; // adjust = max_fps / 2; adjust = 12; /* 11 a 14 */ for (fps = 0; fps <= max_fps; fps++) { if (max_fps == 30) i1 = std_525_60[fps]; else i1 = std_625_50[fps]; i2 = (adjust + 15 * fps) / max_fps; if (fps && !i2) i2 = 1; if (i2 > 14) i2 = 0; //if (i1 != i2) printf("\t%s fps %d, i1=%d, i2=%d\n", (i1 == i2)? " " : "NOK", fps, i1, i2); } } void main(void) { printf ("60 Hz\n"); calc_fps(30); printf ("\n50 Hz\n"); calc_fps(25); } -- Thanks, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
