On Sat, 2009-01-03 at 10:36 +0100, Hans Verkuil wrote:
> Hi Andy,
>
> I've done a quick review:
>
> On Thursday 01 January 2009 23:20:08 Andy Walls wrote:
> > The patch in line below adds a control to the cx18 driver to request
> > AC-3 audio instead of MPEG Layer II. It doesn't quite work yet due to
> > cx18 firmware issues.
> >
> > However, I think I've got the basic control work done and need a review
> > to make sure I didn't muck anything up with the cx2341x or ivtv modules.
> >
> > Of particular concern to me is
> >
> > a) changing the cx2341x "audio_properties" from a u16 to a u32, as this
> > is what rippled down in source code to the to ivtv driver.
> >
> > b) accidentally adding a bogus options or controls to ivtv.
> >
> >
> > The change can also be found at
> >
> > http://linuxtv.org/hg/~awalls/v4l-dvb
> >
> > Regards,
> > Andy
> >
> >
> > # HG changeset patch
> > # User Andy Walls <[email protected]>
> > # Date 1230847351 18000
> > # Node ID e9cf344a6749de5d3778fac3c7114476f7f0b647
> > # Parent 41242777b3d8bb162c65c2d0b2c542417b72d946
> > cx18, cx2341x, ivtv: Add AC-3 audio encoding control to cx18
> >
> > From: Andy Walls <[email protected]>
> >
> > Initial addition of controls to set AC-3 audio encoding for the CX23418 -
> > it does not work yet due to firmware or cx18 driver issues. This change
> > affects the common cx2341x and ivtv modules due to shared structures and
> > common functions.
> >
> > Priority: normal
> >
> > Signed-off-by: Andy Walls <[email protected]>
> >
> > diff -r 41242777b3d8 -r e9cf344a6749
> > linux/drivers/media/video/cx18/cx18-driver.c ---
> > a/linux/drivers/media/video/cx18/cx18-driver.c Thu Jan 01 10:35:06 2009
> > -0500 +++ b/linux/drivers/media/video/cx18/cx18-driver.c Thu Jan 01
> > 17:02:31 2009 -0500 @@ -592,7 +592,8 @@ static int __devinit
> > cx18_init_struct1(s
> > (cx->params.video_temporal_filter_mode << 1) |
> > (cx->params.video_median_filter_type << 2);
> > cx->params.port = CX2341X_PORT_MEMORY;
> > - cx->params.capabilities = CX2341X_CAP_HAS_TS;
> > + cx->params.capabilities = CX2341X_CAP_HAS_TS | CX2341X_CAP_HAS_AC3 |
> > + CX2341X_CAP_HAS_LPCM;
> > init_waitqueue_head(&cx->cap_w);
> > init_waitqueue_head(&cx->mb_apu_waitq);
> > init_waitqueue_head(&cx->mb_cpu_waitq);
> > diff -r 41242777b3d8 -r e9cf344a6749
> > linux/drivers/media/video/cx18/cx18-driver.h ---
> > a/linux/drivers/media/video/cx18/cx18-driver.h Thu Jan 01 10:35:06 2009
> > -0500 +++ b/linux/drivers/media/video/cx18/cx18-driver.h Thu Jan 01
> > 17:02:31 2009 -0500 @@ -413,7 +413,7 @@ struct cx18 {
> >
> > /* dualwatch */
> > unsigned long dualwatch_jiffies;
> > - u16 dualwatch_stereo_mode;
> > + u32 dualwatch_stereo_mode;
> >
> > /* Digitizer type */
> > int digitizer; /* 0x00EF = saa7114 0x00FO = saa7115 0x0106 =
> > mic */
> > diff -r 41242777b3d8 -r e9cf344a6749
> > linux/drivers/media/video/cx18/cx18-fileops.c ---
> > a/linux/drivers/media/video/cx18/cx18-fileops.c Thu Jan 01 10:35:06 2009
> > -0500 +++ b/linux/drivers/media/video/cx18/cx18-fileops.c Thu Jan 01
> > 17:02:31 2009 -0500 @@ -128,10 +128,10 @@ static void
> > cx18_dualwatch(struct cx18 *
> > static void cx18_dualwatch(struct cx18 *cx)
> > {
> > struct v4l2_tuner vt;
> > - u16 new_bitmap;
> > - u16 new_stereo_mode;
> > - const u16 stereo_mask = 0x0300;
> > - const u16 dual = 0x0200;
> > + u32 new_bitmap;
> > + u32 new_stereo_mode;
> > + const u32 stereo_mask = 0x0300;
> > + const u32 dual = 0x0200;
> > u32 h;
> >
> > new_stereo_mode = cx->params.audio_properties & stereo_mask;
> > diff -r 41242777b3d8 -r e9cf344a6749 linux/drivers/media/video/cx2341x.c
> > --- a/linux/drivers/media/video/cx2341x.c Thu Jan 01 10:35:06 2009 -0500
> > +++ b/linux/drivers/media/video/cx2341x.c Thu Jan 01 17:02:31 2009 -0500
> > @@ -1,5 +1,5 @@
> > /*
> > - * cx2341x - generic code for cx23415/6 based devices
> > + * cx2341x - generic code for cx23415/6/8 based devices
> > *
> > * Copyright (C) 2006 Hans Verkuil <[email protected]>
> > *
> > @@ -31,7 +31,7 @@
> > #include <media/v4l2-common.h>
> > #include "compat.h"
> >
> > -MODULE_DESCRIPTION("cx23415/6 driver");
> > +MODULE_DESCRIPTION("cx23415/6/8 driver");
> > MODULE_AUTHOR("Hans Verkuil");
> > MODULE_LICENSE("GPL");
> >
> > @@ -46,6 +46,7 @@ const u32 cx2341x_mpeg_ctrls[] = {
> > V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ,
> > V4L2_CID_MPEG_AUDIO_ENCODING,
> > V4L2_CID_MPEG_AUDIO_L2_BITRATE,
> > + V4L2_CID_MPEG_AUDIO_AC3_BITRATE,
> > V4L2_CID_MPEG_AUDIO_MODE,
> > V4L2_CID_MPEG_AUDIO_MODE_EXTENSION,
> > V4L2_CID_MPEG_AUDIO_EMPHASIS,
> > @@ -95,6 +96,7 @@ static const struct cx2341x_mpeg_params
> > .audio_sampling_freq = V4L2_MPEG_AUDIO_SAMPLING_FREQ_48000,
> > .audio_encoding = V4L2_MPEG_AUDIO_ENCODING_LAYER_2,
> > .audio_l2_bitrate = V4L2_MPEG_AUDIO_L2_BITRATE_224K,
> > + .audio_ac3_bitrate = V4L2_MPEG_AUDIO_AC3_BITRATE_224K,
> > .audio_mode = V4L2_MPEG_AUDIO_MODE_STEREO,
> > .audio_mode_extension = V4L2_MPEG_AUDIO_MODE_EXTENSION_BOUND_4,
> > .audio_emphasis = V4L2_MPEG_AUDIO_EMPHASIS_NONE,
> > @@ -149,6 +151,9 @@ static int cx2341x_get_ctrl(const struct
> > case V4L2_CID_MPEG_AUDIO_L2_BITRATE:
> > ctrl->value = params->audio_l2_bitrate;
> > break;
> > + case V4L2_CID_MPEG_AUDIO_AC3_BITRATE:
> > + ctrl->value = params->audio_ac3_bitrate;
> > + break;
> > case V4L2_CID_MPEG_AUDIO_MODE:
> > ctrl->value = params->audio_mode;
> > break;
> > @@ -257,12 +262,23 @@ static int cx2341x_set_ctrl(struct cx234
> > params->audio_sampling_freq = ctrl->value;
> > break;
> > case V4L2_CID_MPEG_AUDIO_ENCODING:
> > + if (busy)
> > + return -EBUSY;
> > + if (params->capabilities & CX2341X_CAP_HAS_AC3 &&
> > + ctrl->value != V4L2_MPEG_AUDIO_ENCODING_LAYER_2 &&
> > + ctrl->value != V4L2_MPEG_AUDIO_ENCODING_AC3)
> > + return -EINVAL;
>
> This can't be right: if CAP_HAS_AC3 is not set, then it will always
> return -EINVAL.
Ack! You're right. I shouldn't have done this stuff on New Year's Day
- I needed more sleep.
> > params->audio_encoding = ctrl->value;
> > break;
> > case V4L2_CID_MPEG_AUDIO_L2_BITRATE:
> > if (busy)
> > return -EBUSY;
> > params->audio_l2_bitrate = ctrl->value;
> > + break;
> > + case V4L2_CID_MPEG_AUDIO_AC3_BITRATE:
> > + if (busy)
> > + return -EBUSY;
> > + params->audio_ac3_bitrate = ctrl->value;
> > break;
>
> This should test for CAP_HAS_AC3 as well.
I wondered about this, since I added V4L2_CID_MPEG_AUDIO_AC3_BITRATE in
the cx2341x_mpeg_ctrls[] list of controls for CX2341x devices, I wasn't
sure of the user experience for always getting -EINVAL for a listed
control. There is no harm in letting the user app set a control value
that will never be used for CX23415/6 devices.
I didn't want to make an alternate cx2341x_mpeg_ctrls[] list. I'll put
in a check for the CAP_HAS_AC3 and make the control inactive or
read-only, if the CAP isn't set.
> > case V4L2_CID_MPEG_AUDIO_MODE:
> > params->audio_mode = ctrl->value;
> > @@ -483,6 +499,12 @@ int cx2341x_ctrl_query(const struct cx23
> >
> > switch (qctrl->id) {
> > case V4L2_CID_MPEG_AUDIO_ENCODING:
> > + if (params->capabilities & CX2341X_CAP_HAS_AC3)
> > + return v4l2_ctrl_query_fill(qctrl,
> > + V4L2_MPEG_AUDIO_ENCODING_LAYER_2,
> > + V4L2_MPEG_AUDIO_ENCODING_AC3, 1,
> > + default_params.audio_encoding);
> > +
> > return v4l2_ctrl_query_fill(qctrl,
> > V4L2_MPEG_AUDIO_ENCODING_LAYER_2,
> > V4L2_MPEG_AUDIO_ENCODING_LAYER_2, 1,
> > @@ -497,6 +519,12 @@ int cx2341x_ctrl_query(const struct cx23
> > case V4L2_CID_MPEG_AUDIO_L1_BITRATE:
> > case V4L2_CID_MPEG_AUDIO_L3_BITRATE:
> > return -EINVAL;
> > +
> > + case V4L2_CID_MPEG_AUDIO_AC3_BITRATE:
> > + return v4l2_ctrl_query_fill(qctrl,
> > + V4L2_MPEG_AUDIO_AC3_BITRATE_48K,
> > + V4L2_MPEG_AUDIO_AC3_BITRATE_448K, 1,
> > + default_params.audio_ac3_bitrate);
>
> Also needs a test.
OK. Can do.
BTW, The old code fell through to the default case. I note that the L1
& L3 bitrate controls aren't valid, are trapped here, but aren't in the
cx2341x_mpeg_ctrls[] list. I was unsure if a control not in the
cx2341x_mpeg_ctrls[] list would ever come through here.
> > case V4L2_CID_MPEG_AUDIO_MODE_EXTENSION:
> > err = v4l2_ctrl_query_fill_std(qctrl);
> > @@ -672,6 +700,15 @@ const char **cx2341x_ctrl_get_menu(const
> > NULL
> > };
> >
> > + static const char *mpeg_audio_encoding_l2_ac3[] = {
> > + "",
> > + "MPEG-1/2 Layer II",
> > + "",
> > + "",
> > + "AC-3",
> > + NULL
> > + };
> > +
> > static const char *cx2341x_video_spatial_filter_mode_menu[] = {
> > "Manual",
> > "Auto",
> > @@ -712,6 +749,9 @@ const char **cx2341x_ctrl_get_menu(const
> > case V4L2_CID_MPEG_STREAM_TYPE:
> > return (p->capabilities & CX2341X_CAP_HAS_TS) ?
> > mpeg_stream_type_with_ts : mpeg_stream_type_without_ts;
> > + case V4L2_CID_MPEG_AUDIO_ENCODING:
> > + return (p->capabilities & CX2341X_CAP_HAS_AC3) ?
> > + mpeg_audio_encoding_l2_ac3 : v4l2_ctrl_get_menu(id);
> > case V4L2_CID_MPEG_AUDIO_L1_BITRATE:
> > case V4L2_CID_MPEG_AUDIO_L3_BITRATE:
> > return NULL;
> > @@ -731,16 +771,36 @@ const char **cx2341x_ctrl_get_menu(const
> > }
> > EXPORT_SYMBOL(cx2341x_ctrl_get_menu);
> >
> > +/* definitions for audio properties bits 29-28 */
> > +#define CX2341X_AUDIO_ENCDING_METHOD_MPEG 0
> > +#define CX2341X_AUDIO_ENCDING_METHOD_AC3 1
> > +#define CX2341X_AUDIO_ENCDING_METHOD_LPCM 2
>
> ENCDING? You mean ENCODING.
Yes, thanks. Sleep deprivation and not wearing my glasses. 8-)
> > +
> > static void cx2341x_calc_audio_properties(struct cx2341x_mpeg_params
> > *params) {
> > - params->audio_properties = (params->audio_sampling_freq << 0) |
> > - ((3 - params->audio_encoding) << 2) |
> > - ((1 + params->audio_l2_bitrate) << 4) |
> > + params->audio_properties =
> > + (params->audio_sampling_freq << 0) |
> > (params->audio_mode << 8) |
> > (params->audio_mode_extension << 10) |
> > (((params->audio_emphasis == V4L2_MPEG_AUDIO_EMPHASIS_CCITT_J17)
> > ? 3 : params->audio_emphasis) << 12) |
> > (params->audio_crc << 14);
> > +
> > + if ((params->capabilities & CX2341X_CAP_HAS_AC3) &&
> > + params->audio_encoding == V4L2_MPEG_AUDIO_ENCODING_AC3) {
> > + params->audio_properties |=
> > +#if 1
> > + /* Not sure if this MPEG Layer II setting is required */
> > + ((3 - V4L2_MPEG_AUDIO_ENCODING_LAYER_2) << 2) |
> > +#endif
> > + (params->audio_ac3_bitrate << 4) |
> > + (CX2341X_AUDIO_ENCDING_METHOD_AC3 << 28);
> > + } else {
> > + /* Assuming MPEG Layer II */
> > + params->audio_properties |=
> > + ((3 - params->audio_encoding) << 2) |
> > + ((1 + params->audio_l2_bitrate) << 4);
> > + }
> > }
> >
> > int cx2341x_ext_ctrls(struct cx2341x_mpeg_params *params, int busy,
> > @@ -1023,7 +1083,10 @@ void cx2341x_log_status(const struct cx2
> > prefix,
> > cx2341x_menu_item(p, V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ),
> > cx2341x_menu_item(p, V4L2_CID_MPEG_AUDIO_ENCODING),
> > - cx2341x_menu_item(p, V4L2_CID_MPEG_AUDIO_L2_BITRATE),
> > + cx2341x_menu_item(p,
> > + p->audio_encoding == V4L2_MPEG_AUDIO_ENCODING_AC3
> > + ? V4L2_CID_MPEG_AUDIO_AC3_BITRATE
> > + : V4L2_CID_MPEG_AUDIO_L2_BITRATE),
> > cx2341x_menu_item(p, V4L2_CID_MPEG_AUDIO_MODE),
> > p->audio_mute ? " (muted)" : "");
> > if (p->audio_mode == V4L2_MPEG_AUDIO_MODE_JOINT_STEREO)
> > diff -r 41242777b3d8 -r e9cf344a6749
> > linux/drivers/media/video/ivtv/ivtv-driver.h ---
> > a/linux/drivers/media/video/ivtv/ivtv-driver.h Thu Jan 01 10:35:06 2009
> > -0500 +++ b/linux/drivers/media/video/ivtv/ivtv-driver.h Thu Jan 01
> > 17:02:31 2009 -0500 @@ -697,7 +697,7 @@ struct ivtv {
> > u64 vbi_data_inserted; /* number of VBI bytes inserted into
> > the MPEG stream */ u32 last_dec_timing[3]; /* cache last
> > retrieved pts/scr/frame values */ unsigned long dualwatch_jiffies;/*
> > jiffies value of the previous dualwatch check */ - u16
> > dualwatch_stereo_mode; /* current detected dualwatch stereo mode */
> > + u32 dualwatch_stereo_mode; /* current detected dualwatch stereo
> > mode */
> >
> >
> > /* VBI state info */
> > diff -r 41242777b3d8 -r e9cf344a6749
> > linux/drivers/media/video/ivtv/ivtv-fileops.c ---
> > a/linux/drivers/media/video/ivtv/ivtv-fileops.c Thu Jan 01 10:35:06 2009
> > -0500 +++ b/linux/drivers/media/video/ivtv/ivtv-fileops.c Thu Jan 01
> > 17:02:31 2009 -0500 @@ -148,10 +148,10 @@ static void
> > ivtv_dualwatch(struct ivtv *
> > static void ivtv_dualwatch(struct ivtv *itv)
> > {
> > struct v4l2_tuner vt;
> > - u16 new_bitmap;
> > - u16 new_stereo_mode;
> > - const u16 stereo_mask = 0x0300;
> > - const u16 dual = 0x0200;
> > + u32 new_bitmap;
> > + u32 new_stereo_mode;
> > + const u32 stereo_mask = 0x0300;
> > + const u32 dual = 0x0200;
> >
> > new_stereo_mode = itv->params.audio_properties & stereo_mask;
> > memset(&vt, 0, sizeof(vt));
> > diff -r 41242777b3d8 -r e9cf344a6749 linux/include/media/cx2341x.h
> > --- a/linux/include/media/cx2341x.h Thu Jan 01 10:35:06 2009 -0500
> > +++ b/linux/include/media/cx2341x.h Thu Jan 01 17:02:31 2009 -0500
> > @@ -1,5 +1,5 @@
> > /*
> > - cx23415/6 header containing common defines.
> > + cx23415/6/8 header containing common defines.
> >
> > This program is free software; you can redistribute it and/or modify
> > it under the terms of the GNU General Public License as published by
> > @@ -28,6 +28,8 @@ enum cx2341x_cap {
> > enum cx2341x_cap {
> > CX2341X_CAP_HAS_SLICED_VBI = 1 << 0,
> > CX2341X_CAP_HAS_TS = 1 << 1,
> > + CX2341X_CAP_HAS_AC3 = 1 << 2,
> > + CX2341X_CAP_HAS_LPCM = 1 << 3,
> > };
> >
> > struct cx2341x_mpeg_params {
> > @@ -47,11 +49,12 @@ struct cx2341x_mpeg_params {
> > enum v4l2_mpeg_audio_sampling_freq audio_sampling_freq;
> > enum v4l2_mpeg_audio_encoding audio_encoding;
> > enum v4l2_mpeg_audio_l2_bitrate audio_l2_bitrate;
> > + enum v4l2_mpeg_audio_ac3_bitrate audio_ac3_bitrate;
> > enum v4l2_mpeg_audio_mode audio_mode;
> > enum v4l2_mpeg_audio_mode_extension audio_mode_extension;
> > enum v4l2_mpeg_audio_emphasis audio_emphasis;
> > enum v4l2_mpeg_audio_crc audio_crc;
> > - u16 audio_properties;
> > + u32 audio_properties;
> > u16 audio_mute;
> >
> > /* video */
>
> Regards,
>
> Hans
Thanks for heading off that disaster of a patch.
Regards,
Andy
_______________________________________________
ivtv-devel mailing list
[email protected]
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
