Em Tue, 4 Aug 2009 16:19:46 -0400 Michael Krufky <mkrufky@xxxxxxxxxxxxxx> escreveu: > On Tue, Aug 4, 2009 at 3:58 PM, Alex Deucher<alexdeucher@xxxxxxxxx> wrote: > > On Tue, Aug 4, 2009 at 3:50 PM, Michael Krufky<mkrufky@xxxxxxxxxxxxxx> wrote: > >> On Tue, Aug 4, 2009 at 3:47 PM, Alex Deucher<alexdeucher@xxxxxxxxx> wrote: > >>> On Tue, Aug 4, 2009 at 3:33 PM, Michael Krufky<mkrufky@xxxxxxxxxxx> wrote: > >>>> Mauro, > >>>> > >>>> Please pull from: > >>>> > >>>> http://kernellabs.com/hg/~mkrufky/cx23885 > >>>> > >>>> for the following fixes: > >>>> > >>>> - cx23885: Enable mplayer pvr:// usage > >>> > >>> I'm not too familiar with mplayer's v4l support, but why not fix > >>> mplayer rather than adding a fake audio interface to the driver. > >>> Wouldn't that potentially confuse users or other apps? > >> > >> Thats a good question, Alex. > >> > >> The answer, for now, is conformity amongst the v4l2 drivers. > >> > >> Mplayer has been around for a good long time, and any v4l2 mpeg > >> encoder that doesnt do this is broken in the vraious userspace > >> applications. > >> > >> I agree that we should fix userspace also -- but fix the kernel first, > >> so that older userspace works as-is. > > > > er... yeah, but you are re-enforcing broken behavior, rather than > > "fixing" more drivers, why not submit a mplayer patch and tell users > > they need a newer version of mplayer/etc. to work with their card? > > Otherwise we'll end up with tons of drivers with fake audio interfaces > > and mplayer will never get fixed and users will complain that the > > audio interface doesn't work. > > You don't really have the full picture here, Alex. The applications > expect to see an audio input, and rightfully so. > > This particular driver doesn't expose all of the functionality that is > available from the raw video device onto the encoder device. > > Little by little, we are fixing up the cx23885-417 driver to fully > expose all featuresets, but in the meanwhile, we do what is needed to > make things work. > > The problem is not mplayer, the real problem is incomplete analog > video (and analog audio) support in the cx23885 driver, itself. As > THAT support improves, you will see small hacks like this disappear. There are some separate issues that came with this patch: 1) V4L2 API (chapter 1.5) states that: Drivers must implement all input ioctls when the device has one or more inputs, all output ioctls when the device has one or more outputs. When the device has any audio inputs or outputs the driver must set the V4L2_CAP_AUDIO flag in the struct v4l2_capability returned by the VIDIOC_QUERYCAP ioctl. My understanding is that, except for webcams without any audio input, all other V4L2 devices should implement VIDIOC_G_AUDIO, VIDIOC_ENUMAUDIO and VIDIOC_S_AUDIO. So, your patch should have implemented VIDIOC_S_AUDIO as well. 2) Based on the above understanding, your comments are wrong, since the device has one audio input (otherwise, no audio would be streamed). So, it is not mplayer that requires those functions, but V4L2 API. So, please remove the comments. The same fix applies to pvrusb2 comments. 3) On this piece of code: + strncpy(vin->name, "CX23885 Audio", 14); + vin->capability = V4L2_AUDCAP_STEREO; the "magic" size above is very ugly. Please find another way for it, like: #define CX23885_INPUT_NAME "CX23885 Audio" strncpy(vin->name, CX23885_INPUT_NAME, sizeof(CX23885_INPUT_NAME) + 1); 4) mplayer should be fixed to not require audio inputs for devices without V4L2_CAP_AUDIO. I just tested it here with a webcam, where I have this parameter at .mplayer/config: tv = "driver=v4l2:device=/dev/video0:norm=PAL-M:chanlist=us-bcast:alsa=1:adevice=hw.1:audiorate=32000:immediatemode=0:amode=1" With a device without audio cap: $ v4l2-ctl -D Driver info: Driver name : em28xx Card type : Silvercrest Webcam 1.3mpix Bus info : usb-0000:00:1d.7-7 Driver version: 258 Capabilities : 0x05000041 Video Capture Sliced VBI Capture Read/Write Streaming it is still requesting for audio input, so mplayer has a small non-compliance with V4L2 API. 5) There is one trouble with VIDIOC_G_AUDIO and VIDIOC_ENUMAUDIO: As stated at V4L2 API, at chapter 6.2.10, added on 2003-06-19: 3. The audio input and output interface was found to be incomplete. Previously the VIDIOC_G_AUDIO ioctl would enumerate the available audio inputs. An ioctl to determine the current audio input, if more than one combines with the current video input, did not exist. So VIDIOC_G_AUDIO was renamed to VIDIOC_G_AUDIO_OLD, this ioctl will be removed in the future. The VIDIOC_ENUMAUDIO ioctl was added to enumerate audio inputs, while VIDIOC_G_AUDIO now reports the current audio input. There are very few drivers that are respecting this change. On almost all drivers, VIDIOC_G_AUDIO is still working like VIDIOC_G_AUDIO_OLD. For example, this is what bttv driver is doing: static int bttv_g_audio(struct file *file, void *priv, struct v4l2_audio *a) { if (unlikely(a->index)) return -EINVAL; strcpy(a->name, "audio"); return 0; } The same V4L2 violation is found at au0828, av7110, cx18, cx231xx, cx23885, cx88, dsbr100, em28xx, ivtv, mxb, radio-tea5764, saa7134, sn9c102. It is correct on: gspca (yet, it assumes that all webcams have audio), hdpvr, pvrusb2, radio-aimslab, radio-aztech, radio-cadet, radio-gemtek, radio-maestro, radio-maxiradio, radio-mr800, radio-rtrack2, radio-sf16fmi, radio-sf16fmr2, radio-si470x, radio-terratec, radio-trust, radio-typhoon, radio-zoltrix, tea575x-tuner, usbvision. For sure all drivers should behave the same for VIDIOC_G_AUDIO. So, we need to decide if we'll keep VIDIOC_G_AUDIO_OLD behavior, or honor the API change stated on 2003. No matter what decision we'll take, this will have impacts at userspace applications. 6) Since VIDIOC_ENUMAUDIO is mandatory when V4L2_CAP_AUDIO, several drivers are violating the API, since this ioctl exists only at cx18, gspca, hdpvr, ivtv, pvrusb2 and sn9c102. So, or we should change V4L2 API to state this as optional ioctl or deprecate it (this would make sense if we decide that VIDIOC_G_AUDIO should use the old behavior), or we should implement it on almost all drivers. As VIDIOC_ENUMAUDIO and VIDIOC_G_AUDIO are very close, in terms of implementation, maybe we can merge those two at v4l2-ioctl, to simplify a little bit the driver's code. Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
