| Message ID | 20160427080928.GC22469@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Dan, Em Wed, 27 Apr 2016 11:09:28 +0300 Dan Carpenter <dan.carpenter@oracle.com> 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. Regards, Mauro > > Fixes: 363d79f1d5bd ('[media] tw686x: Don't go past array') > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c > index d2a0147..7b87f27 100644 > --- a/drivers/media/pci/tw686x/tw686x-video.c > +++ b/drivers/media/pci/tw686x/tw686x-video.c > @@ -64,12 +64,12 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps) > unsigned int i; > > if (std & V4L2_STD_525_60) { > - if (fps > ARRAY_SIZE(std_525_60)) > - fps = 30; > + if (fps >= ARRAY_SIZE(std_525_60)) > + fps = ARRAY_SIZE(std_525_60) - 1; > i = std_525_60[fps]; > } else { > - if (fps > ARRAY_SIZE(std_625_50)) > - fps = 25; > + if (fps >= ARRAY_SIZE(std_625_50)) > + fps = ARRAY_SIZE(std_625_50) - 1; > i = std_625_50[fps]; > } >
On Wed, Apr 27, 2016 at 07:31:59AM -0300, Mauro Carvalho Chehab wrote: > 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. Fine fine. I buy that rationale. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Wed, 27 Apr 2016 07:31:59 -0300 Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu: > Hi Dan, > > Em Wed, 27 Apr 2016 11:09:28 +0300 > Dan Carpenter <dan.carpenter@oracle.com> 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); }
diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c index d2a0147..7b87f27 100644 --- a/drivers/media/pci/tw686x/tw686x-video.c +++ b/drivers/media/pci/tw686x/tw686x-video.c @@ -64,12 +64,12 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps) unsigned int i; if (std & V4L2_STD_525_60) { - if (fps > ARRAY_SIZE(std_525_60)) - fps = 30; + if (fps >= ARRAY_SIZE(std_525_60)) + fps = ARRAY_SIZE(std_525_60) - 1; i = std_525_60[fps]; } else { - if (fps > ARRAY_SIZE(std_625_50)) - fps = 25; + if (fps >= ARRAY_SIZE(std_625_50)) + fps = ARRAY_SIZE(std_625_50) - 1; i = std_625_50[fps]; }
The > ARRAY_SIZE() should be >= ARRAY_SIZE(). Also this is a slightly unrelated cleanup but I replaced the magic numbers 30 and 25 with ARRAY_SIZE() - 1. Fixes: 363d79f1d5bd ('[media] tw686x: Don't go past array') Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html