Message ID | 20160427080928.GC22469@mwanda (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1avKYA-0004I7-Pq; Wed, 27 Apr 2016 08:10:22 +0000 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.76/mailfrontend-6) with esmtp id 1avKY8-0004Nr-53; Wed, 27 Apr 2016 10:10:22 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753541AbcD0IJv (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Wed, 27 Apr 2016 04:09:51 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:42693 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752586AbcD0IJr (ORCPT <rfc822;linux-media@vger.kernel.org>); Wed, 27 Apr 2016 04:09:47 -0400 Received: from userv0022.oracle.com (userv0022.oracle.com [156.151.31.74]) by aserp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id u3R89hD9027295 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 27 Apr 2016 08:09:44 GMT Received: from aserv0122.oracle.com (aserv0122.oracle.com [141.146.126.236]) by userv0022.oracle.com (8.14.4/8.13.8) with ESMTP id u3R89gaQ007134 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 27 Apr 2016 08:09:42 GMT Received: from abhmp0012.oracle.com (abhmp0012.oracle.com [141.146.116.18]) by aserv0122.oracle.com (8.13.8/8.13.8) with ESMTP id u3R89cZW015376; Wed, 27 Apr 2016 08:09:39 GMT Received: from mwanda (/154.0.139.178) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 27 Apr 2016 01:09:38 -0700 Date: Wed, 27 Apr 2016 11:09:28 +0300 From: Dan Carpenter <dan.carpenter@oracle.com> To: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>, Mauro Carvalho Chehab <mchehab@osg.samsung.com> Cc: linux-media@vger.kernel.org, kernel-janitors@vger.kernel.org Subject: [patch] [media] tw686x: off by one bugs in tw686x_fields_map() Message-ID: <20160427080928.GC22469@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.24 (2015-08-30) X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2016.4.27.80315 X-PMX-Spam: Gauge=IIIIIIIII, Probability=9%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, MSGID_ADDED_BY_MTA 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_1100_1199 0, BODY_SIZE_2000_LESS 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, INVALID_MSGID_NO_FQDN 0, NO_URI_HTTPS 0, SINGLE_URI_IN_BODY 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __CD 0, __CP_MEDIA_BODY 0, __CP_URI_IN_BODY 0, __CT 0, __CT_TEXT_PLAIN 0, __DATE_TZ_RU 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILING_LIST 0, __MIME_TEXT_ONLY 0, __MIME_VERSION 0, __MULTIPLE_RCPTS_CC_X2 0, __SANE_MSGID 0, __SINGLE_URI_TEXT 0, __TO_MALFORMED_2 0, __URI_IN_BODY 0, __URI_NO_WWW 0, __URI_NS , __URI_WITH_PATH 0, __USER_AGENT 0' |
Commit Message
Dan Carpenter
April 27, 2016, 8:09 a.m. UTC
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
Comments
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]; }