[MPlayer-dev-eng] [PATCH] Radio support for MPlayer
Reynaldo H. Verdejo Pinochet
reynaldo at opendot.cl
Sun Jul 9 08:44:56 CEST 2006
On Mon, Jul 03, 2006 at 12:43:37PM +0700, Vladimir Voroshilov wrote:
> Hi, All
Hi Vladimir
> This is next (i hope last) update for my patch.
> In this version 'capture' keyword in movie URL was added (to make
> possible set up audio device names in config file without
> starting capture automatically in this case).
> Threads have been removed.
>
Nice
> I hope that this version is final.
Who knows ;)
> Can anybody review patch and make decision about commit?
Lets see.
>
> P.S. please make attention on buffer_time, is this solution acceptable?
I skimmed through it, seems ok but guess you will need to post an
isolated patch for that to be reviewed and (if) commited.
> Index: mplayer.c
> ===================================================================
> --- mplayer.c (revision 18889)
> +++ mplayer.c (working copy)
> [...]
> @@ -4392,6 +4396,36 @@
> }
> brk_cmd = 1;
> } break;
> +#ifdef USE_RADIO
> + case MP_CMD_RADIO_STEP_CHANNEL : {
> + if (demuxer->stream->type==STREAMTYPE_RADIO) {
> + int v = cmd->args[0].v.i;
> + if(v > 0)
> + radio_step_channel(demuxer->stream, RADIO_CHANNEL_HIGHER);
> + else
> + radio_step_channel(demuxer->stream, RADIO_CHANNEL_LOWER);
> + if (radio_get_channel_name(demuxer->stream)) {
> + set_osd_msg(OSD_MSG_RADIO_CHANNEL,1,osd_duration,
> + MSGTR_OSDChannel, radio_get_channel_name(demuxer->stream));
> + //vo_osd_changed(OSDTYPE_SUBTITLE);
I dont get why you didnt remove that comment.
> + }
> + }
> + } break;
> + case MP_CMD_RADIO_SET_CHANNEL : {
> + if (demuxer->stream->type== STREAMTYPE_RADIO) {
> + radio_set_channel(demuxer->stream, cmd->args[0].v.s);
> + if (radio_get_channel_name(demuxer->stream)) {
> + set_osd_msg(OSD_MSG_RADIO_CHANNEL,1,osd_duration,
> + MSGTR_OSDChannel, radio_get_channel_name(demuxer->stream));
> + //vo_osd_changed(OSDTYPE_SUBTITLE);
Same here
> [...]
> Index: cfg-common.h
> [...]
> Index: help/help_mp-ru.h
> [...]
> Index: help/help_mp-en.h
> [...]
> Index: libmpdemux/ai_oss.c
> ===================================================================
> --- libmpdemux/ai_oss.c (revision 18889)
> +++ libmpdemux/ai_oss.c (working copy)
> @@ -3,7 +3,7 @@
>
> #include "config.h"
>
> -#if defined(USE_TV) && (defined(HAVE_TV_V4L) || defined(HAVE_TV_V4L2)) && defined(USE_OSS_AUDIO)
> +#if (defined(USE_RADIO) || defined(USE_TV)) && (defined(HAVE_RADIO_V4L) || defined(HAVE_RADIO_V4L2) || defined(HAVE_TV_V4L) || defined(HAVE_TV_V4L2))
>
This lime was moved by diego on r18927, besides, you seem to be missing
an '&& USE_OSS_AUDIO'.
> #include <string.h> /* strerror */
> #include <fcntl.h>
> Index: libmpdemux/ai_alsa.c
> ===================================================================
> --- libmpdemux/ai_alsa.c (revision 18889)
> +++ libmpdemux/ai_alsa.c (working copy)
> @@ -4,7 +4,7 @@
>
> #include "config.h"
>
> -#if defined(USE_TV) && (defined(HAVE_TV_V4L) || defined(HAVE_TV_V4L2)) && defined(HAVE_ALSA9)
> +#if (defined(USE_RADIO) || defined(USE_TV)) && (defined(HAVE_RADIO_V4L) || defined(HAVE_RADIO_V4L2) || defined(HAVE_TV_V4L) || defined(HAVE_TV_V4L2)) && defined(HAVE_ALSA9)
Same here, conditional compilation was moved into the build system.
>
> #include <alsa/asoundlib.h>
> #include "audio_in.h"
> @@ -52,7 +52,7 @@
> rate = err;
> ai->samplerate = rate;
>
> - ai->alsa.buffer_time = 1000000;
> + //buffer_time was set in audio_in_init
The coment has no meaning if you remove the above line, either remove
both or comment the first instead of removing. btw, youre better of
making this change on an isolated patch.
> ai->alsa.buffer_time = snd_pcm_hw_params_set_buffer_time_near(ai->alsa.handle, params,
> ai->alsa.buffer_time, 0);
> assert(ai->alsa.buffer_time >= 0);
> Index: libmpdemux/Makefile
> [...]
> Index: libmpdemux/stream_radio.c
> ===================================================================
> --- libmpdemux/stream_radio.c (revision 0)
> +++ libmpdemux/stream_radio.c (revision 0)
> @@ -0,0 +1,1025 @@
> +
> +#include "config.h"
> +
> +/*
> + * Radio support by Vladimir Voroshilov.
> + *
> + * Based on tv.c and tvi_v4l2.c code.
> + *
> + * Abilities:
> + * * Listening v4l compatible radio cards using line-in or
> + * similar cable
> + * * Grabbing audio data using -ao pcm or -dumpaudio
> + * (must be compiled with --enable-radio-capture).
> + */
If this is under GPL please add the corresponding note here.
> [...]
> + *
> + */
> +char* radio_param_device="/dev/radio0";
> +char** radio_param_channels=NULL;
> +int radio_param_volume=100;
> +int radio_param_preload=1;
> +char* radio_param_adevice=NULL;
> +int radio_param_arate=44100;/// -rawaudio rate parameter must be set to the same value
> +int radio_param_achannels=2;
You dont need the =NULLs here but it doesnt really matter.
> [...]
> +typedef struct radio_priv_s {
> + int radio_fd; ///radio device descriptor
> + int frac;/// fraction value (see comment to init_frac)
Please try harder to make your indentation constant ;)
> [...]
> +
> + mp_msg(MSGT_RADIO, MSGL_INFO, MSGTR_RADIO_ChannelNamesDetected);
> + priv->radio_channel_list = malloc(sizeof(radio_channels_t));
Woha! you didnt cast malloc, kudos for you, +10 MP points from the
janitors guilt! ;)
> [...]
> + struct v4l2_control control;
> +
> + memset(&control,0,sizeof(control));
> + control.id=V4L2_CID_AUDIO_MUTE;
> + control.value = (mute?1:0);
> + if (ioctl(priv->radio_fd, VIDIOC_S_CTRL, &control)<0){
> + mp_msg(MSGT_RADIO,MSGL_ERR,MSGTR_RADIO_SetMuteFailed,strerror(errno));
is this really an error ? do all cards support to be mutted ? if not
this should be a warning instead, im not really sure but guess you can
make an aditional check against EINVAL/ENOTTY if needed.
> [...]
> +
> + memset(&qctrl,0,sizeof(qctrl));
> + qctrl.id = V4L2_CID_AUDIO_VOLUME;
> + if (ioctl(priv->radio_fd, VIDIOC_QUERYCTRL, &qctrl) < 0) {
> + mp_msg(MSGT_RADIO, MSGL_ERR, MSGTR_RADIO_QueryControlFailed,strerror(errno));
> + return STREAM_ERROR;
Same here.
> [...]
> + qctrl.id = V4L2_CID_AUDIO_VOLUME;
> + if (ioctl(priv->radio_fd, VIDIOC_QUERYCTRL, &qctrl) < 0) {
> + mp_msg(MSGT_RADIO, MSGL_ERR, MSGTR_RADIO_QueryControlFailed,strerror(errno));
Same here.
> [...]
> + struct video_tuner tuner;
> + memset(&tuner,0,sizeof(tuner));
> + if (ioctl(priv->radio_fd, VIDIOCGTUNER, &tuner) <0){
> + priv->frac=16;
> + mp_msg(MSGT_RADIO,MSGL_WARN,MSGTR_RADIO_GetTunerFailed,strerror(errno),priv->frac);
> + return STREAM_OK;
STREAM_OK ? see... I mean, I see youre only spitting out a warning but isnt
this more important than a failed set_volume ?
> [...]
> + * \brief mute card
> + * \param mute 1-mute, 0-unmute
> + * \return STREAM_OK if success, STREAM_ERROR otherwise
> + */
> +static int set_mute(radio_priv_t* priv,int mute){
> + struct video_audio audio;
> + memset(&audio,0,sizeof(audio));
> + audio.flags = (mute?VIDEO_AUDIO_MUTE:0);
> + if (ioctl(priv->radio_fd, VIDIOCSAUDIO, &audio)<0){
> + mp_msg(MSGT_RADIO,MSGL_ERR,MSGTR_RADIO_SetMuteFailed,strerror(errno));
> + return STREAM_ERROR;
I forgot to tell you before, but maybe you can just tell MP to make a
soft mute here ;) I maintain that maybe an 'ERROR' its too much.
Btw, cant you code this as an special case of the set_volume function
bellow ?
> + }
> + return STREAM_OK;
> +}
> +
> +/*****************************************************************
> + * \brief set volume on radio card
> + * \param volume volume level (0..100)
> + * \return STREAM_OK if success, STREAM_ERROR otherwise
> + */
> +static int set_volume(radio_priv_t* priv,int volume){
> + struct video_audio audio;
> +
> [...]
> [...]
> + * NOTE: radio card MUST be muted when audio_in_setup called. I don't know why :-/ ,
> + * otherwise we will get no sound
> + */
> +static int init_audio(radio_priv_t *priv)
> +{
> + int is_oss=1;
> + int seconds=2;
> + char* tmp;
> + if (priv->audio_inited) return 1;
> +
> + /* do_capture==0 this that mplayer does not start with capture keyword*/
It may be my english but is that comment supposed to mean something ?
> [...]
> + mp_msg(MSGT_RADIO, MSGL_ERR, MSGTR_RADIO_AudioInInitFailed,strerror(errno));
> + }
> +#endif
> +
> +
> + audio_in_set_device(&priv->audio_in, radio_param_adevice);
> + audio_in_set_channels(&priv->audio_in, radio_param_achannels);
> + audio_in_set_samplerate(&priv->audio_in, radio_param_arate);
> +
> + /* during audio_in_setup radio card MUST be muted. I don't know why :-/ */
To protect our ears I guess ;)
> [...]
> + }
> + }else
> + mp_msg(MSGT_RADIO, MSGL_ERR, MSGTR_RADIO_ChangeChannelNoChannelList);
> + return(1);
> +}
A cosmetic note, you are found to use a mix of 'return X' and 'return (X)'.
with no reason whatsoever.
> Index: libmpdemux/stream_radio.h
> [...]
> Index: libmpdemux/audio_in.c
> [...]
> Index: libmpdemux/ai_alsa1x.c
> ===================================================================
> --- libmpdemux/ai_alsa1x.c (revision 18889)
> +++ libmpdemux/ai_alsa1x.c (working copy)
> @@ -4,7 +4,7 @@
>
> #include "config.h"
>
> -#if defined(USE_TV) && (defined(HAVE_TV_V4L) || defined(HAVE_TV_V4L2)) && defined(HAVE_ALSA1X)
> +#if (defined(USE_RADIO) || defined(USE_TV)) && (defined(HAVE_RADIO_V4L) || defined(HAVE_RADIO_V4L2) || defined(HAVE_TV_V4L) || defined(HAVE_TV_V4L2)) && defined(HAVE_ALSA1X)
Same comment applies, this has been moved.
I swear that while reviewing your patch I spoted a few ignored return
values from non trivial nor void returning functions, I forgot to mark
them somewhere while writing this answer but please keep in mind ..
Best regards and thanks for your work.
Reynaldo
ps: if you fix it and no one says otherwise i will commit this.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20060709/62a4a573/attachment.pgp>
More information about the MPlayer-dev-eng
mailing list