Re: [PATCH] cx88: implement sharpness control
On 04/09/2010 06:33 AM, Mauro Carvalho Chehab wrote: > You're not adjusting the sharpness. Instead, you're changing the vertical tap > filter, > and just for the even frames, plus the notch filter. > Tricky, and you're probably affecting the sharpness, but on an indirect and > non-linear > way, as you're adjusting different measures. No, I actually change the luma peak filter, which can add a peak of up to about 6 dB centered at the chroma subcarrier frequency. So, it does increase sharpness, and horizontally, not vertically. I did test the patch before submitting it. Also, there is code in the patch that sets the parameter for the odd field as well. So, in fact, this control affects three registers: two for the peak filter, and a third one for the notch filter. > If this is really needed, it would be better to break it into two controls > (one for the notch filter and another for the vertical tap filter). Yes, that could possibly have been a good idea to have a separate control for the notch filter. It was just easier to implement this way, and I think there is no existing control that would be well suited for the notch filter. Since both parameters do affect sharpness in some way, I implemented it as a single control, rather than creating a separate new one. > Also, the "side effect" is not good: if you're using those bits, your code > should assure > that no other part of the driver will touch on the used bits, and that the > device will > be initialized with the default standard. That is correct, and I did note that it would be addressed later if the general idea of implementing the control is accepted. Fortunately, the registers are changed elsewhere at only about two places. By the way, did you have a look at the other two cx88 patches (containing minor fixes only) I sent at the same time ? -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] cx88: implement sharpness control
istva...@mailbox.hu wrote: > This patch adds support for V4L2_CID_SHARPNESS by changing the luma peak > filter and notch filter. It can be set in the range 0 to 9, with 0 being > the original and default mode. > One minor problem is that other code that sets the registers being used > (for example when switching TV channels) could reset the control to the > default. This could be avoided by making changes so that the bits used > to implement this control are not overwritten. > > Signed-off-by: Istvan Varga > > + case V4L2_CID_SHARPNESS: > + /* use 4xFsc or square pixel notch filter */ > + cx_andor(MO_HTOTAL, 0x1800, (ctl->value & 1) << 11); > + /* 0b000, 0b100, 0b101, 0b110, or 0b111 */ > + value = (ctl->value < 2 ? 0 : (((ctl->value + 6) & 0x0E) << 6)); > + /* needs to be set for both fields */ > + cx_andor(MO_FILTER_EVEN, mask, value); > + break; You're not adjusting the sharpness. Instead, you're changing the vertical tap filter, and just for the even frames, plus the notch filter. Tricky, and you're probably affecting the sharpness, but on an indirect and non-linear way, as you're adjusting different measures. This doesn't seem right, at least on my eyes. If this is really needed, it would be better to break it into two controls (one for the notch filter and another for the vertical tap filter). Also, the "side effect" is not good: if you're using those bits, your code should assure that no other part of the driver will touch on the used bits, and that the device will be initialized with the default standard. -- Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] cx88: implement sharpness control
This patch adds support for V4L2_CID_SHARPNESS by changing the luma peak
filter and notch filter. It can be set in the range 0 to 9, with 0 being
the original and default mode.
One minor problem is that other code that sets the registers being used
(for example when switching TV channels) could reset the control to the
default. This could be avoided by making changes so that the bits used
to implement this control are not overwritten.
Signed-off-by: Istvan Varga
diff -r -d -N -U4 v4l-dvb-a79dd2ae4d0e.old/linux/drivers/media/video/cx88/cx88-video.c v4l-dvb-a79dd2ae4d0e/linux/drivers/media/video/cx88/cx88-video.c
--- v4l-dvb-a79dd2ae4d0e.old/linux/drivers/media/video/cx88/cx88-video.c 2010-03-23 03:39:52.0 +0100
+++ v4l-dvb-a79dd2ae4d0e/linux/drivers/media/video/cx88/cx88-video.c 2010-03-23 19:07:26.0 +0100
@@ -220,9 +220,24 @@
.off = 0,
.reg = MO_UV_SATURATION,
.mask = 0x00ff,
.shift = 0,
- },{
+ }, {
+ .v = {
+ .id= V4L2_CID_SHARPNESS,
+ .name = "Sharpness",
+ .minimum = 0,
+ .maximum = 9,
+ .default_value = 0x0,
+ .type = V4L2_CTRL_TYPE_INTEGER,
+ },
+ .off = 0,
+ /* NOTE: the value is converted and written to both even
+ and odd registers in the code */
+ .reg = MO_FILTER_ODD,
+ .mask = 7 << 7,
+ .shift = 7,
+ }, {
.v = {
.id= V4L2_CID_CHROMA_AGC,
.name = "Chroma AGC",
.minimum = 0,
@@ -300,8 +315,9 @@
V4L2_CID_HUE,
V4L2_CID_AUDIO_VOLUME,
V4L2_CID_AUDIO_BALANCE,
V4L2_CID_AUDIO_MUTE,
+ V4L2_CID_SHARPNESS,
V4L2_CID_CHROMA_AGC,
V4L2_CID_COLOR_KILLER,
0
};
@@ -1187,8 +1203,13 @@
break;
case V4L2_CID_AUDIO_VOLUME:
ctl->value = 0x3f - (value & 0x3f);
break;
+ case V4L2_CID_SHARPNESS:
+ ctl->value = (value & 0x0380) >> 6;
+ ctl->value = (ctl->value < 8 ? 0 : (ctl->value - 6))
+ | ((cx_read(MO_HTOTAL) & 0x0800) >> 11);
+ break;
default:
ctl->value = ((value + (c->off << c->shift)) & c->mask) >> c->shift;
break;
}
@@ -1246,8 +1267,16 @@
value=(value*0x5a)/0x7f<<8|value;
}
mask=0x;
break;
+ case V4L2_CID_SHARPNESS:
+ /* use 4xFsc or square pixel notch filter */
+ cx_andor(MO_HTOTAL, 0x1800, (ctl->value & 1) << 11);
+ /* 0b000, 0b100, 0b101, 0b110, or 0b111 */
+ value = (ctl->value < 2 ? 0 : (((ctl->value + 6) & 0x0E) << 6));
+ /* needs to be set for both fields */
+ cx_andor(MO_FILTER_EVEN, mask, value);
+ break;
case V4L2_CID_CHROMA_AGC:
/* Do not allow chroma AGC to be enabled for SECAM */
value = ((ctl->value - c->off) << c->shift) & c->mask;
if (core->tvnorm & V4L2_STD_SECAM && value)
