[FFmpeg-devel] [PATCH] ALSA for libavdevice

[Michael Niedermayer]

On Tue, Dec 09, 2008 at 08:48:00PM +0100, Nicolas George wrote:
> Hi. Thanks for your review.
> 
> Le nonidi 19 frimaire, an CCXVII, Diego Biurrun a ?crit?:
> > Why do you add it at this weird point in the Makefile?
> 
> Some confusion. Is it better?
> 
> > missing multiple inclusion guard prefix
> 
> Right, fixed.
> 
[...]
> +/**
> + * @file alsa-audio-common.c
> + * ALSA input and output: common code
> + * @author Luca Abeni ( lucabe72 email it )
> + * @author Nicolas George ( nicolas george normalesup org )

> + */ 

trailing whitespace


> +
> +#include "libavformat/avformat.h"
> +#include <alsa/asoundlib.h>
> +
> +#include "alsa-audio.h"
> +

> +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


> +{

> +    switch(codec_id) {
> +        case CODEC_ID_PCM_S16LE:
> +            return SND_PCM_FORMAT_S16_LE;
> +        case CODEC_ID_PCM_S16BE:
> +            return SND_PCM_FORMAT_S16_BE;
> +        case CODEC_ID_PCM_S8:
> +            return SND_PCM_FORMAT_S8;
> +        default:
> +            return SND_PCM_FORMAT_UNKNOWN;

putting each case only on a single line and vertically aligned should be
more readable


> +    }
> +}
> +

> +int ff_alsa_open(AVFormatContext *ctx, int mode)
> +{
> +    AlsaData *s = ctx->priv_data;
> +    const char *audio_device;
> +    int res, flags = 0;
> +    snd_pcm_format_t format;
> +    snd_pcm_t *h;
> +    snd_pcm_hw_params_t *hw_params;
> +    snd_pcm_uframes_t buffer_size, period_size;
> +
> +    if (ctx->filename[0] == 0) {
> +        audio_device = "default";
> +    } else {
> +        audio_device = ctx->filename;
> +    }
> +
> +    if (s->codec_id == CODEC_ID_NONE)
> +        s->codec_id = DEFAULT_CODEC_ID;
> +    format = alsa_codec_id(s->codec_id);
> +    if (format == SND_PCM_FORMAT_UNKNOWN) {
> +        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


[...]
> +int ff_alsa_xrun_recover(AVFormatContext *s1, int err)
> +{
> +    AlsaData *s = s1->priv_data;
> +    snd_pcm_t *handle = s->h;
> +

> +    av_log(s1, AV_LOG_WARNING, "XRUN!!!\n");

This message can be improved i think

[...]
> +static int audio_read_close(AVFormatContext *s1)
> +{
> +    AlsaData *s = s1->priv_data;
> +
> +    ff_alsa_close(s);
> +    return 0;
> +}

silly wraper function


[...]
> +    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


[...]
> +static int audio_read_packet(AVFormatContext *s1, AVPacket *pkt)
> +{
> +    AlsaData *s = s1->priv_data;
> +    int res;
> +
> +    if (av_new_packet(pkt, s->period_size) < 0) {
> +        return AVERROR(EIO);
> +    }
> +    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);
> +        }
> +    }
> +    pkt->size = res * s->frame_size;

> +    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


[...]
> +static int audio_write_trailer(AVFormatContext *s1)
> +{
> +    AlsaData *s = s1->priv_data;
> +
> +    ff_alsa_close(s);
> +    return 0;
> +}

silly wraper function

[...]
> +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


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

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- 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/20081209/da7c8e89/attachment.pgp>