On Tue, Aug 4, 2009 at 7:40 PM, Mauro Carvalho Chehab<mchehab@xxxxxxxxxxxxx> wrote: > 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. Thank you for the feedback, Mauro -- I see your point. We will have to take some time to come up with a better solution. I really wish that people would refrain from hijacking email threads -- it makes it difficult to keep any conversation on track. If that person should be reading this email right now, I ask again, next time to please start your OWN email thread and stop interfering with others. Things like this make it difficult to hold such discussions in the public. Please stay on-topic, or start a new thread. Back to the point, I will have to assess how much work needs to be done in order to comply with everything that you stated above. Is there any middle ground that we can agree on, so that we can break this large problem up into several smaller problems? Thanks & regards, Mike -- 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
