tw686x: off by one bugs in tw686x_fields_map()

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.

index d2a0147..7b87f27 100644
@@ -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];
 	}
 

Hi Dan,

Em Wed, 27 Apr 2016 11:09:28 +0300
Dan Carpenter <[email protected]> escreveu:

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/?idEc175c4ae9695d6d2f30a45ab7f3866cfac184b

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

Thanks,
Mauro

Fine fine. I buy that rationale.

regards,
dan carpenter

Em Wed, 27 Apr 2016 07:31:59 -0300
Mauro Carvalho Chehab <[email protected]> escreveu:

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, i2
	    fps 21, i1, i2
	    fps 22, i1, i2
	    fps 23, i1, i2
	    fps 24, i1, i2
	    fps 25, i1, i2
	    fps 26, i1, i2
	    fps 27, i1, i2
	    fps 28, i1, i2
	NOK fps 29, i1=0, i2
	    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, i2
	    fps 17, i1, i2
	    fps 18, i1, i2
	    fps 19, i1, i2
	    fps 20, i1, i2
	    fps 21, i1, i2
	    fps 22, i1, i2
	    fps 23, i1, i2
	    fps 24, i1, i2
	    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

Instead of using two tables to estimate the temporal decimation factor, use a formula. This allows to get the closest fps, with sounds better than the current tables.

Compile-tested only.

---
 drivers/media/pci/tw686x/tw686x-video.c | 34 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

index 253e10823ba3..0210fa304e4c 100644
@@ -50,28 +50,18 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
 		0x01041041, 0x01104411, 0x01111111, 0x04444445, 0x04511445,
 		0x05145145, 0x05151515, 0x05515455, 0x05551555, 0x05555555
 	};
-
-	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
-	};
-
-	unsigned int i;
-
-	if (std & V4L2_STD_525_60) {
-		if (fps >= ARRAY_SIZE(std_525_60))
-			fps = 30;
-		i = std_525_60[fps];
-	} else {
-		if (fps >= ARRAY_SIZE(std_625_50))
-			fps = 25;
-		i = std_625_50[fps];
-	}
+	unsigned int i, max_fps;
+
+	if (std & V4L2_STD_525_60)
+		max_fps = 30;
+	else
+		max_fps = 25;
+
+	i = DIV_ROUND_CLOSEST(15 * fps, max_fps);
+	if (!i)
+		i = 1;	/* Min possible fps */
+	else if (i > 14)
+		i = 0;	/* fps = max_fps */
 
 	return map[i];
 }

2.5.5

Hmm....

Em Wed, 27 Apr 2016 08:01:19 -0300
Mauro Carvalho Chehab <[email protected]> escreveu:

Please discard this patch. It is wrong.

I found the datasheet for this device at:

	http://www.starterkit.ru/html/doc/tw6869-ds.pdf

Based on what it is said on page 50, it seems that it doesn't use a decimation filter, but, instead, it just discards some fields in a way that the average fps will be reduced.

So, the actual frame rate is given by the number of enabled bits that are written to VIDEO_FIELD_CTRL[vc->ch] and are at the valid bitmask range, e. g:

index  0, map = 0x00000000, 30 fps (60Hz), 25 fps (50Hz), output all fields
index  1, map = 0x80000006, 2 fps (60Hz), 2 fps (50Hz)
index  2, map = 0x80018006, 4 fps (60Hz), 4 fps (50Hz)
index  3, map = 0x80618006, 6 fps (60Hz), 6 fps (50Hz)
index  4, map = 0x81818186, 8 fps (60Hz), 8 fps (50Hz)
index  5, map = 0x86186186, 10 fps (60Hz), 8 fps (50Hz)
index  6, map = 0x86619866, 12 fps (60Hz), 10 fps (50Hz)
index  7, map = 0x86666666, 14 fps (60Hz), 12 fps (50Hz)
index  8, map = 0x9999999e, 16 fps (60Hz), 14 fps (50Hz)
index  9, map = 0x99e6799e, 18 fps (60Hz), 16 fps (50Hz)

index 10, map = 0x9e79e79e, 20 fps (60Hz), 16 fps (50Hz) index 11, map = 0x9e7e7e7e, 22 fps (60Hz), 18 fps (50Hz) index 12, map = 0x9fe7f9fe, 24 fps (60Hz), 20 fps (50Hz) index 13, map = 0x9ffe7ffe, 26 fps (60Hz), 22 fps (50Hz) index 14, map = 0x9ffffffe, 28 fps (60Hz), 24 fps (50Hz)

This was done by using the enclosed program.

---

#include <stdio.h>

static const unsigned int map[15] = {
                0x00000000, 0x00000001, 0x00004001, 0x00104001, 0x00404041,
               	0x01041041, 0x01104411, 0x01111111, 0x04444445, 0x04511445,
                0x05145145, 0x05151515, 0x05515455, 0x05551555, 0x05555555
};

unsigned int count_bits(unsigned int bits, unsigned int max)

{
	unsigned int i, c= 0;

	for (i = 0; i < max; i++)
		if ((1 << i) & bits)
			c++;

	return c;
}

void calc_map(unsigned int i)

{
	unsigned int m, fps_30, fps_25;

	m = map[i] << 1;
	m |= m << 1;

	fps_30 = count_bits(m, 30);
	fps_25 = count_bits(m, 25);

	if (m)
		m |= 1 << 31;
	else {
		fps_30 = 30;
		fps_25 = 25;
	}

	printf("index %2i, map = 0x%08x, %d fps (60Hz), %d fps (50Hz)%s\n",
		i, m, fps_30, fps_25,
		i == 0 ? ", output all fields" : ""
		);
}

int main(void)

{
	unsigned int i;

	printf ("\nmap:\n");
	for (i = 0; i < 15; i++)
		calc_map(i);

	return 0;
}

There are some issues with the old code:
1) it uses two static tables;
2) some values for 50Hz standards are wrong;
3) it doesn't store the real framerate.

This patch fixes the above issues.

Compile-tested only.

---

PS.: With this patch, it should be easy to add support for VIDIOC_G_PARM and VIDIOC_S_PARM, as vc->fps will now store the real frame rate, with should be used when returning from those functions.

 drivers/media/pci/tw686x/tw686x-video.c | 100 +++++++++++++++++++++++---------
 1 file changed, 73 insertions(+), 27 deletions(-)

index 0210fa304e4c..b247a7b4ddd8 100644
@@ -43,43 +43,89 @@ static const struct tw686x_format formats[] = {
 	}
 };
 
-static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
+static const unsigned int fps_map[15] = {
+	/*
+	 * bit 31 enables selecting the field control register
+	 * bits 0-29 are a bitmask with fields that will be output.
+	 * For NTSC (and PAL-M, PAL-60), all 30 bits are used.
+	 * For other PAL standards, only the first 25 bits are used.
+	 */
+	0x00000000, /* output all fields */
+	0x80000006, /* 2 fps (60Hz), 2 fps (50Hz) */
+	0x80018006, /* 4 fps (60Hz), 4 fps (50Hz) */
+	0x80618006, /* 6 fps (60Hz), 6 fps (50Hz) */
+	0x81818186, /* 8 fps (60Hz), 8 fps (50Hz) */
+	0x86186186, /* 10 fps (60Hz), 8 fps (50Hz) */
+	0x86619866, /* 12 fps (60Hz), 10 fps (50Hz) */
+	0x86666666, /* 14 fps (60Hz), 12 fps (50Hz) */
+	0x9999999e, /* 16 fps (60Hz), 14 fps (50Hz) */
+	0x99e6799e, /* 18 fps (60Hz), 16 fps (50Hz) */
+	0x9e79e79e, /* 20 fps (60Hz), 16 fps (50Hz) */
+	0x9e7e7e7e, /* 22 fps (60Hz), 18 fps (50Hz) */
+	0x9fe7f9fe, /* 24 fps (60Hz), 20 fps (50Hz) */
+	0x9ffe7ffe, /* 26 fps (60Hz), 22 fps (50Hz) */
+	0x9ffffffe, /* 28 fps (60Hz), 24 fps (50Hz) */
+};
+
+static unsigned int tw686x_real_fps(unsigned int index, unsigned int max_fps)
+{
+	unsigned int i, bits, c = 0;
+
+	if (!index || index >= ARRAY_SIZE(fps_map))
+		return max_fps;
+
+	bits = fps_map[index];
+	for (i = 0; i < max_fps; i++)
+		if ((1 << i) & bits)
+			c++;
+
+	return c;
+}
+
+static unsigned int tw686x_fps_idx(unsigned int fps, unsigned int max_fps)
 {
-	static const unsigned int map[15] = {
-		0x00000000, 0x00000001, 0x00004001, 0x00104001, 0x00404041,
-		0x01041041, 0x01104411, 0x01111111, 0x04444445, 0x04511445,
-		0x05145145, 0x05151515, 0x05515455, 0x05551555, 0x05555555
-	};
-	unsigned int i, max_fps;
-
-	if (std & V4L2_STD_525_60)
-		max_fps = 30;
-	else
-		max_fps = 25;
-
-	i = DIV_ROUND_CLOSEST(15 * fps, max_fps);
-	if (!i)
-		i = 1;	/* Min possible fps */
-	else if (i > 14)
-		i = 0;	/* fps = max_fps */
-
-	return map[i];
+	unsigned int idx, real_fps;
+	int delta;
+
+	/* First guess */
+	idx = (12 + 15 * fps) / max_fps;
+
+	/* Minimal possible framerate is 2 frames per second */
+	if (!idx)
+		return 1;
+
+	/* Check if the difference is bigger than abs(1) and adjust */
+	real_fps = tw686x_real_fps(idx, max_fps);
+	delta = real_fps - fps;
+	if (delta < -1)
+		idx++;
+	else if (delta > 1)
+		idx--;
+
+	/* Max framerate */
+	if (idx >= 15)
+		return 0;
+
+	return idx;
 }
 
 static void tw686x_set_framerate(struct tw686x_video_channel *vc,
 				 unsigned int fps)
 {
-	unsigned int map;
+	unsigned int i, max_fps;
 
 	if (vc->fps == fps)
 		return;
 
-	map = tw686x_fields_map(vc->video_standard, fps) << 1;
-	map |= map << 1;
-	if (map > 0)
-		map |= BIT(31);
-	reg_write(vc->dev, VIDEO_FIELD_CTRL[vc->ch], map);
-	vc->fps = fps;
+	if (vc->video_standard & V4L2_STD_525_60)
+		max_fps = 30;
+	else
+		max_fps = 25;
+
+	i = tw686x_fps_idx(fps, max_fps);
+
+	reg_write(vc->dev, VIDEO_FIELD_CTRL[vc->ch], fps_map[i]);
+	vc->fps = tw686x_real_fps(i, max_fps);
 }
 
 static const struct tw686x_format *format_by_fourcc(unsigned int fourcc)

2.5.5

Instead of using two tables to estimate the temporal decimation factor, use a formula. This allows to get the closest fps, with sounds better than the current tables.

Compile-tested only.

[media] tw686x: cleanup the fps estimation code

There are some issues with the old code:
1) it uses two static tables;
2) some values for 50Hz standards are wrong;
3) it doesn't store the real framerate.

This patch fixes the above issues.

Compile-tested only.

-

v3: Patch v2 were actually a diff patch against PATCH v1. Fold the two patches in one.

PS.: With this patch, it should be easy to add support for VIDIOC_G_PARM and VIDIOC_S_PARM, as vc->fps will now store the real frame rate, with should be used when returning from those functions.

---
 drivers/media/pci/tw686x/tw686x-video.c | 110 +++++++++++++++++++++-----------
 1 file changed, 73 insertions(+), 37 deletions(-)

index 253e10823ba3..b247a7b4ddd8 100644
@@ -43,53 +43,89 @@ static const struct tw686x_format formats[] = {
 	}
 };
 
-static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
+static const unsigned int fps_map[15] = {
+	/*
+	 * bit 31 enables selecting the field control register
+	 * bits 0-29 are a bitmask with fields that will be output.
+	 * For NTSC (and PAL-M, PAL-60), all 30 bits are used.
+	 * For other PAL standards, only the first 25 bits are used.
+	 */
+	0x00000000, /* output all fields */
+	0x80000006, /* 2 fps (60Hz), 2 fps (50Hz) */
+	0x80018006, /* 4 fps (60Hz), 4 fps (50Hz) */
+	0x80618006, /* 6 fps (60Hz), 6 fps (50Hz) */
+	0x81818186, /* 8 fps (60Hz), 8 fps (50Hz) */
+	0x86186186, /* 10 fps (60Hz), 8 fps (50Hz) */
+	0x86619866, /* 12 fps (60Hz), 10 fps (50Hz) */
+	0x86666666, /* 14 fps (60Hz), 12 fps (50Hz) */
+	0x9999999e, /* 16 fps (60Hz), 14 fps (50Hz) */
+	0x99e6799e, /* 18 fps (60Hz), 16 fps (50Hz) */
+	0x9e79e79e, /* 20 fps (60Hz), 16 fps (50Hz) */
+	0x9e7e7e7e, /* 22 fps (60Hz), 18 fps (50Hz) */
+	0x9fe7f9fe, /* 24 fps (60Hz), 20 fps (50Hz) */
+	0x9ffe7ffe, /* 26 fps (60Hz), 22 fps (50Hz) */
+	0x9ffffffe, /* 28 fps (60Hz), 24 fps (50Hz) */
+};
+
+static unsigned int tw686x_real_fps(unsigned int index, unsigned int max_fps)
+{
+	unsigned int i, bits, c = 0;
+
+	if (!index || index >= ARRAY_SIZE(fps_map))
+		return max_fps;
+
+	bits = fps_map[index];
+	for (i = 0; i < max_fps; i++)
+		if ((1 << i) & bits)
+			c++;
+
+	return c;
+}
+
+static unsigned int tw686x_fps_idx(unsigned int fps, unsigned int max_fps)
 {
-	static const unsigned int map[15] = {
-		0x00000000, 0x00000001, 0x00004001, 0x00104001, 0x00404041,
-		0x01041041, 0x01104411, 0x01111111, 0x04444445, 0x04511445,
-		0x05145145, 0x05151515, 0x05515455, 0x05551555, 0x05555555
-	};
-
-	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
-	};
-
-	unsigned int i;
-
-	if (std & V4L2_STD_525_60) {
-		if (fps >= ARRAY_SIZE(std_525_60))
-			fps = 30;
-		i = std_525_60[fps];
-	} else {
-		if (fps >= ARRAY_SIZE(std_625_50))
-			fps = 25;
-		i = std_625_50[fps];
-	}
-
-	return map[i];
+	unsigned int idx, real_fps;
+	int delta;
+
+	/* First guess */
+	idx = (12 + 15 * fps) / max_fps;
+
+	/* Minimal possible framerate is 2 frames per second */
+	if (!idx)
+		return 1;
+
+	/* Check if the difference is bigger than abs(1) and adjust */
+	real_fps = tw686x_real_fps(idx, max_fps);
+	delta = real_fps - fps;
+	if (delta < -1)
+		idx++;
+	else if (delta > 1)
+		idx--;
+
+	/* Max framerate */
+	if (idx >= 15)
+		return 0;
+
+	return idx;
 }
 
 static void tw686x_set_framerate(struct tw686x_video_channel *vc,
 				 unsigned int fps)
 {
-	unsigned int map;
+	unsigned int i, max_fps;
 
 	if (vc->fps == fps)
 		return;
 
-	map = tw686x_fields_map(vc->video_standard, fps) << 1;
-	map |= map << 1;
-	if (map > 0)
-		map |= BIT(31);
-	reg_write(vc->dev, VIDEO_FIELD_CTRL[vc->ch], map);
-	vc->fps = fps;
+	if (vc->video_standard & V4L2_STD_525_60)
+		max_fps = 30;
+	else
+		max_fps = 25;
+
+	i = tw686x_fps_idx(fps, max_fps);
+
+	reg_write(vc->dev, VIDEO_FIELD_CTRL[vc->ch], fps_map[i]);
+	vc->fps = tw686x_real_fps(i, max_fps);
 }
 
 static const struct tw686x_format *format_by_fourcc(unsigned int fourcc)

2.5.5

Hi Mauro,

Thanks a lot for the patch.

Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Em Mon, 27 Jun 2016 12:45:38 -0300
Ezequiel Garcia <[email protected]> escreveu:

Good!

I would be expecting a maximum difference a little bigger than 1 Hz.

No. This is the same table that was already in the code:

static const unsigned int map[15] = {
                0x00000000, 0x00000001, 0x00004001, 0x00104001, 0x00404041,
                0x01041041, 0x01104411, 0x01111111, 0x04444445, 0x04511445,
                0x05145145, 0x05151515, 0x05515455, 0x05551555, 0x05555555
};

Except that the calculus that used to be there to set bit 31 to 1 on everything except map[0] and the code that makes it set two FPS at the same time were pre-calculated, e. g. I run this code locally to generate the new table:

	map = tw686x_fields_map(vc->video_standard, fps) << 1;
	map |= map << 1;
	if (map > 0)
		map |= BIT(31);

There, bit 31 = 0 disables the frame filtering. bit 31 = 1 enables it.

Each bit at the 0-29 bitrange means one of the 30 frames received. If equal to 1, the frame is sent; if equal to 0, it is not sent.

So, the first value, for example: 0x80000006 has 2 consecutive bits set, so the mean frame rate would be 2Hz. The next value there is 0x0x80018006, with has 4 bits selected, so mean fps is 4Hz, and so on.

As the original table always sets 2 consecutive frames at the same time, I suspect that this is a requirement to avoid interlacing issues.

So, the code I used to generate the table always allocate 2 consecutive bits each time.

I suspect that such the table was built assuming that there are 30 bits, but, on 50Hz, only 25 bits are used. So, it is sub-optimal for 50 Hz. It means that the frames are not equally spaced.

If you want, I can construct a table for 50Hz that would produce better results.

Thanks,
Mauro

Actually, it's working fine on both 50Hz and 60Hz standards. I tested PAL-Nc using my modifications, and I had an small error in the hweight_long usage. My bad!

So... now I think it's working properly, for 50Hz the requested FPS matches the actual FPS, and for 60Hz there's a slight difference:

* 24 FPS gives 23.50 FPS
* 22 FPS gives 21.66 FPS
* 20 FPS gives 20 FPS
* 10 FPS gives 10 FPS
* 6 FPS give 5 FPS
* 2 FPS gives 1.66 FPS

Output FPS values are those measured by qv4l2.

Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Em Wed, 27 Apr 2016 09:00:49 -0300
Mauro Carvalho Chehab <[email protected]> escreveu:

Ok, I think I got this right this time. See the enclosed code.

It produces the fps register map, with each fps associated to both 60Hz and 50Hz standard, plus it replaces the tables by a calculus.

If my code is right, there are some values on the current tables that are wrong.

See below. I'll submit an updated patch soon.

---
Program results:

FPS map:

	index  0, map = 0x00000000, 30 fps (60Hz), 25 fps (50Hz), output all fields
	index  1, map = 0x80000006, 2 fps (60Hz), 2 fps (50Hz)
	index  2, map = 0x80018006, 4 fps (60Hz), 4 fps (50Hz)
	index  3, map = 0x80618006, 6 fps (60Hz), 6 fps (50Hz)
	index  4, map = 0x81818186, 8 fps (60Hz), 8 fps (50Hz)
	index  5, map = 0x86186186, 10 fps (60Hz), 8 fps (50Hz)
	index  6, map = 0x86619866, 12 fps (60Hz), 10 fps (50Hz)
	index  7, map = 0x86666666, 14 fps (60Hz), 12 fps (50Hz)
	index  8, map = 0x9999999e, 16 fps (60Hz), 14 fps (50Hz)
	index  9, map = 0x99e6799e, 18 fps (60Hz), 16 fps (50Hz)
	index 10, map = 0x9e79e79e, 20 fps (60Hz), 16 fps (50Hz)
	index 11, map = 0x9e7e7e7e, 22 fps (60Hz), 18 fps (50Hz)
	index 12, map = 0x9fe7f9fe, 24 fps (60Hz), 20 fps (50Hz)
	index 13, map = 0x9ffe7ffe, 26 fps (60Hz), 22 fps (50Hz)
	index 14, map = 0x9ffffffe, 28 fps (60Hz), 24 fps (50Hz)

60 Hz
Requested fps 0, table 0 (30 fps, delta 30), calculus 1 (2 fps, delta 2) DIFFERENT! Requested fps 1, table 1 (2 fps, delta 1), calculus 1 (2 fps, delta 1) Requested fps 2, table 1 (2 fps, delta 0), calculus 1 (2 fps, delta 0) Requested fps 3, table 1 (2 fps, delta -1), calculus 1 (2 fps, delta -1) Requested fps 4, table 2 (4 fps, delta 0), calculus 2 (4 fps, delta 0) Requested fps 5, table 2 (4 fps, delta -1), calculus 2 (4 fps, delta -1) Requested fps 6, table 3 (6 fps, delta 0), calculus 3 (6 fps, delta 0) Requested fps 7, table 3 (6 fps, delta -1), calculus 3 (6 fps, delta -1) Requested fps 8, table 4 (8 fps, delta 0), calculus 4 (8 fps, delta 0) Requested fps 9, table 4 (8 fps, delta -1), calculus 4 (8 fps, delta -1) Requested fps 10, table 5 (10 fps, delta 0), calculus 5 (10 fps, delta 0) Requested fps 11, table 5 (10 fps, delta -1), calculus 5 (10 fps, delta -1) Requested fps 12, table 6 (12 fps, delta 0), calculus 6 (12 fps, delta 0) Requested fps 13, table 6 (12 fps, delta -1), calculus 6 (12 fps, delta -1) Requested fps 14, table 7 (14 fps, delta 0), calculus 7 (14 fps, delta 0) Requested fps 15, table 7 (14 fps, delta -1), calculus 7 (14 fps, delta -1) Requested fps 16, table 8 (16 fps, delta 0), calculus 8 (16 fps, delta 0) Requested fps 17, table 8 (16 fps, delta -1), calculus 8 (16 fps, delta -1) Requested fps 18, table 9 (18 fps, delta 0), calculus 9 (18 fps, delta 0) Requested fps 19, table 9 (18 fps, delta -1), calculus 9 (18 fps, delta -1) Requested fps 20, table 10 (20 fps, delta 0), calculus 10 (20 fps, delta 0) Requested fps 21, table 10 (20 fps, delta -1), calculus 10 (20 fps, delta -1) Requested fps 22, table 11 (22 fps, delta 0), calculus 11 (22 fps, delta 0) Requested fps 23, table 11 (22 fps, delta -1), calculus 11 (22 fps, delta -1) Requested fps 24, table 12 (24 fps, delta 0), calculus 12 (24 fps, delta 0) Requested fps 25, table 12 (24 fps, delta -1), calculus 12 (24 fps, delta -1) Requested fps 26, table 13 (26 fps, delta 0), calculus 13 (26 fps, delta 0) Requested fps 27, table 13 (26 fps, delta -1), calculus 13 (26 fps, delta -1) Requested fps 28, table 14 (28 fps, delta 0), calculus 14 (28 fps, delta 0) Requested fps 29, table 0 (30 fps, delta 1), calculus 14 (28 fps, delta -1) DIFFERENT! Requested fps 30, table 0 (30 fps, delta 0), calculus 0 (30 fps, delta 0)

50 Hz
Requested fps 0, table 0 (25 fps, delta 25), calculus 1 (2 fps, delta 2) DIFFERENT! Requested fps 1, table 1 (2 fps, delta 1), calculus 1 (2 fps, delta 1) Requested fps 2, table 1 (2 fps, delta 0), calculus 1 (2 fps, delta 0) Requested fps 3, table 2 (4 fps, delta 1), calculus 2 (4 fps, delta 1) Requested fps 4, table 3 (6 fps, delta 2), calculus 2 (4 fps, delta 0) DIFFERENT! Requested fps 5, table 3 (6 fps, delta 1), calculus 3 (6 fps, delta 1) Requested fps 6, table 4 (8 fps, delta 2), calculus 3 (6 fps, delta 0) DIFFERENT! Requested fps 7, table 4 (8 fps, delta 1), calculus 4 (8 fps, delta 1) Requested fps 8, table 5 (8 fps, delta 0), calculus 5 (8 fps, delta 0) Requested fps 9, table 5 (8 fps, delta -1), calculus 5 (8 fps, delta -1) Requested fps 10, table 6 (10 fps, delta 0), calculus 6 (10 fps, delta 0) Requested fps 11, table 7 (12 fps, delta 1), calculus 7 (12 fps, delta 1) Requested fps 12, table 7 (12 fps, delta 0), calculus 7 (12 fps, delta 0) Requested fps 13, table 8 (14 fps, delta 1), calculus 8 (14 fps, delta 1) Requested fps 14, table 8 (14 fps, delta 0), calculus 8 (14 fps, delta 0) Requested fps 15, table 9 (16 fps, delta 1), calculus 9 (16 fps, delta 1) Requested fps 16, table 10 (16 fps, delta 0), calculus 10 (16 fps, delta 0) Requested fps 17, table 10 (16 fps, delta -1), calculus 10 (16 fps, delta -1) Requested fps 18, table 11 (18 fps, delta 0), calculus 11 (18 fps, delta 0) Requested fps 19, table 11 (18 fps, delta -1), calculus 11 (18 fps, delta -1) Requested fps 20, table 12 (20 fps, delta 0), calculus 12 (20 fps, delta 0) Requested fps 21, table 13 (22 fps, delta 1), calculus 13 (22 fps, delta 1) Requested fps 22, table 13 (22 fps, delta 0), calculus 13 (22 fps, delta 0) Requested fps 23, table 14 (24 fps, delta 1), calculus 14 (24 fps, delta 1) Requested fps 24, table 14 (24 fps, delta 0), calculus 14 (24 fps, delta 0) Requested fps 25, table 0 (25 fps, delta 0), calculus 0 (25 fps, delta 0)

---

Program:

#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
};

static const unsigned int map[15] = {
                0x00000000, 0x00000001, 0x00004001, 0x00104001, 0x00404041,
               	0x01041041, 0x01104411, 0x01111111, 0x04444445, 0x04511445,
                0x05145145, 0x05151515, 0x05515455, 0x05551555, 0x05555555
};

unsigned int real_rate(unsigned int index, unsigned int max)

{
	unsigned int i, bits, c = 0;

	if (!index)
		return max;

	bits = map[index] << 1;
	bits |= bits << 1;

	for (i = 0; i < max; i++)
		if ((1 << i) & bits)
			c++;

	return c;
}

void calc_map(unsigned int i)

{
	unsigned int m, fps_30, fps_25;

	fps_30 = real_rate(i, 30);
	fps_25 = real_rate(i, 25);

	m = map[i] << 1;
	m |= m << 1;

	if (m)
		m |= 1 << 31;
	else {
		fps_30 = 30;
		fps_25 = 25;
	}

	printf("\tindex %2i, map = 0x%08x, %d fps (60Hz), %d fps (50Hz)%s\n",
		i, m, fps_30, fps_25,
		i == 0 ? ", output all fields" : ""
		);
}

unsigned int get_index(unsigned int fps, unsigned int max_fps)

{
	unsigned int idx, real_fps;
	int delta;

	if (real_fps == max_fps)
		return 0;

	if (fps < 2)
		return 1;

	/* First guess */
	idx = (12 + 15 * fps) / max_fps;

	/* Check if the difference is bigger than abs(1) and adjust */
	real_fps = real_rate(idx, max_fps);
	delta = real_fps - fps;
	if (delta < -1)
		idx++;
	else if (delta > 1)
		idx--;

	if (idx >= 15)
		return 0;

	return idx;
}

void calc_fps(unsigned int max_fps)

{
	unsigned int i1, i2, fps, adjust;
	unsigned int fps1, fps2;

	for (fps = 0; fps <= max_fps; fps++) {
		if (max_fps == 30)
			i1 = std_525_60[fps];
		else
			i1 = std_625_50[fps];

		i2 = get_index(fps, max_fps);

		fps1 = real_rate(i1, max_fps);
		fps2 = real_rate(i2, max_fps);

		printf("\tRequested fps %d, table %d (%d fps, delta %d), calculus %d (%d fps, delta %d)%s\n",
			fps,
			i1, fps1, fps1 - fps,
			i2, fps2, fps2 - fps,
			(i1 != i2) ? " DIFFERENT!": "");
	}
}

int main(void)

{
	unsigned int i;

	printf ("FPS map:\n");
	for (i = 0; i < 15; i++)
		calc_map(i);

	printf ("\n60 Hz\n");
	calc_fps(30);

	printf ("\n50 Hz\n");
	calc_fps(25);

	return 0;
}