[FFmpeg-devel] [PATCH] ALSA for libavdevice

[Michael Niedermayer]

On Fri, Dec 12, 2008 at 06:38:47PM +0100, Nicolas George wrote:
> Hi. Thanks for your review.
> 
> 
> Le nonidi 19 frimaire, an CCXVII, Michael Niedermayer a ?crit?:
> > > + */ 
> > trailing whitespace
> 
> Fixed.
> 
> > > +static snd_pcm_format_t alsa_codec_id(int codec_id)
> > ff2alsa_codec_id() or a similar name that makes it clear that its not the
> > inverse
> 
> Renamed to codec_id_to_pcm_format.
> 
> > > +    switch(codec_id) {
> > > +        case CODEC_ID_PCM_S16LE:
> > > +            return SND_PCM_FORMAT_S16_LE;
> > putting each case only on a single line and vertically aligned should be
> > more readable
> 
> It seems to be.
> 
> 
> > > +        av_log(NULL, AV_LOG_ERROR, "sample format %x is not supported\n", s->codec_id);
> > null is a poor context and should be avoided where possible, there may
> > be some cases where its too messy to avoid but here ctx can be used
> 
> Fixed.
> 
> > > +    av_log(s1, AV_LOG_WARNING, "XRUN!!!\n");
> > This message can be improved i think
> 
> "ALSA buffer underrun.\n"
> 
> > > +static int audio_read_close(AVFormatContext *s1)
> > > +{
> > > +    AlsaData *s = s1->priv_data;
> > > +
> > > +    ff_alsa_close(s);
> > > +    return 0;
> > > +}
> > silly wraper function
> 
> Fixed.
> 
> > > +    s->sample_rate = ap->sample_rate;
> > > +    s->channels = ap->channels;
> > > +    s->codec_id = ap->audio_codec_id;
> > redundant, they are placed in st->codec already
> 
> Fixed, at the cost of a few more parameters to ff_alsa_open.
> 
> > > +    pkt->pts = av_gettime();   /* FIXME: We might need something better... */
> > yes, this really needs to be improved, this is unacceptable unless alsa
> > completely lacks functionality to do better
> 
> Changed to use snd_pcm_htimestamp.
> 
> > > +static int audio_write_trailer(AVFormatContext *s1)
> > > +{
> > > +    AlsaData *s = s1->priv_data;
> > > +
> > > +    ff_alsa_close(s);
> > > +    return 0;
> > > +}
> > silly wraper function
> 
> Ditto.
> 
> > > +extern int ff_alsa_open(AVFormatContext *s, int mode);
> > > +extern int ff_alsa_close(AlsaData *s);
> > > +extern int ff_alsa_xrun_recover(AVFormatContext *s1, int err);
> > the extern is unneeded and they lack doxy commets explaining what they do
> 
> Done.
> 
> Regarding Luca's remarks: I added Benoit Fouet in the header. As for the
> format detection, I do not think it would be worth the additional code.
> 
> Regards,
[...]
> +static int audio_read_header(AVFormatContext *s1, AVFormatParameters *ap)
> +{
> +    AlsaData *s = s1->priv_data;
> +    AVStream *st;
> +    int ret;
> +    unsigned int sample_rate;
> +    int codec_id;
> +    snd_pcm_sw_params_t *sw_params;
> +
> +    if (ap->sample_rate <= 0 || ap->channels <= 0) {
> +        av_log(s1, AV_LOG_ERROR, "Bad sample rate %d or channels number %d\n",
> +                   ap->sample_rate, ap->channels);
> +
> +        return AVERROR(EIO);
> +    }
> +
> +    st = av_new_stream(s1, 0);
> +    if (st == NULL) {
> +        av_log(s1, AV_LOG_ERROR, "Cannot add stream\n");
> +
> +        return AVERROR(ENOMEM);
> +    }
> +    sample_rate = ap->sample_rate;
> +    codec_id = ap->audio_codec_id;
> +
> +    ret = ff_alsa_open(s1, SND_PCM_STREAM_CAPTURE, &sample_rate, ap->channels,
> +        &codec_id);
> +    if (ret < 0) {
> +        return AVERROR(EIO);
> +    }
> +
> +    ret = snd_pcm_sw_params_malloc(&sw_params);
> +    if (ret < 0) {
> +        av_log(s1, AV_LOG_ERROR, "cannot allocate software parameters structure (%s)\n",
> +            snd_strerror(ret));
> +        snd_pcm_close(s->h);
> +        return AVERROR(EIO);
> +    }
> +
> +    snd_pcm_sw_params_current(s->h, sw_params);
> +
> +    ret = snd_pcm_sw_params_set_tstamp_mode(s->h, sw_params,
> +        SND_PCM_TSTAMP_ENABLE);
> +    if (ret < 0) {
> +        av_log(s1, AV_LOG_ERROR, "cannot set ALSA timestamp mode (%s)\n",
> +            snd_strerror(ret));
> +        snd_pcm_close(s->h);
> +        return AVERROR(EIO);
> +    }
> +
> +    ret = snd_pcm_sw_params(s->h, sw_params);
> +    snd_pcm_sw_params_free(sw_params);
> +     if (ret < 0) {
> +        av_log(s1, AV_LOG_ERROR, "cannot install ALSA software parameters (%s)\n",
> +            snd_strerror(ret));
> +        snd_pcm_close(s->h);
> +         return AVERROR(EIO);
> +     }

"snd_pcm_close(s->h); return AVERROR(EIO);" could be after a fail:


[...]
> +    while ((res = snd_pcm_readi(s->h, pkt->data, pkt->size / s->frame_size)) < 0) {
> +        if (res == -EAGAIN) {
> +            pkt->size = 0;
> +            av_free_packet(pkt);
> +
> +            return AVERROR(EAGAIN);
> +        }
> +        if (ff_alsa_xrun_recover(s1, res) < 0) {
> +                av_log(s1, AV_LOG_ERROR, "Alsa read error: %s\n",
> +                                   snd_strerror(res));
> +                av_free_packet(pkt);
> +
> +                return AVERROR(EIO);
> +        }
> +    }

the indention is odd


> +
> +    snd_pcm_htimestamp(s->h, &ts_delay, &timestamp);
> +    ts_delay += res;

> +    pkt->pts = (int64_t)timestamp.tv_sec * 1000000 + timestamp.tv_nsec / 1000;
> +    pkt->pts -= (int64_t)ts_delay * 1000000 / st->codec->sample_rate;

i think the timebase should be choosen so that one of these doesnt need to be
rounded


[...]
> +typedef struct {
> +    snd_pcm_t *h;
> +    int frame_size; /* preferred size for reads and writes */
> +    int period_size; /* bytes per sample * channels */
> +} AlsaData;

not doxygen compatible comments


> +
> +/**
> + * Opens an ALSA PCM.
> + *
> + * @param s media file handle
> + * @param mode either SND_PCM_STREAM_CAPTURE or SND_PCM_STREAM_PLAYBACK

> + * @param sample_rate sample rate
> + * @param channels number of channels
> + * @param codec_id CodecID

sample_rate and codec_id are pointers, are they inputs? outputs?
i think this should be clarified

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- 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/ffmpeg-devel/attachments/20081213/d70dbd9b/attachment.pgp>