[FFmpeg-devel] [PATCH] Add support for sndio to libavdevice

[Diego Biurrun]

On Mon, Aug 02, 2010 at 07:42:28PM -0400, Brad wrote:
> 
> Please provide any feedback.

Here you go..

> --- Changelog	(revision 24666)
> +++ Changelog	(working copy)
> @@ -27,9 +27,9 @@
>  - SubRip subtitle file muxer and demuxer
>  - Chinese AVS encoding via libxavs
>  - ffprobe -show_packets option added
> +- sndio support for playback and record
>  
>  
> -
>  version 0.6:

The empty line was there on purpose.

> --- libavdevice/sndio.c	(revision 0)
> +++ libavdevice/sndio.c	(revision 0)
> @@ -0,0 +1,299 @@
> +
> +#include "config.h"
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include <sndio.h>
> +
> +#include "libavutil/log.h"
> +#include "libavcodec/avcodec.h"
> +#include "libavformat/avformat.h"
> +
> +
> +typedef struct {
> +    struct sio_hdl *hdl;
> +    int sample_rate;
> +    int channels;
> +    int bps;
> +    enum CodecID codec_id;
> +    int buffer_size;
> +    uint8_t *buffer;
> +    int buffer_ptr;
> +    long long hwpos, softpos;
> +} AudioData;
> +
> +static void movecb(void *addr, int delta)
> +{
> +    AudioData *s = addr;
> +
> +    s->hwpos += delta * s->channels * s->bps;
> +}
> +
> +static int audio_open(AVFormatContext *s1, int is_output, const char *audio_device)

long line

> +    if (hdl == NULL) {

if (!hdl) {

more below

> +    sio_initpar(&par);
> +    par.bits = 16;
> +    par.sig = 1;
> +    par.le = SIO_LE_NATIVE;

Align the =.

> +    if (par.bits != 16 || par.sig != 1 || par.le != SIO_LE_NATIVE ||
> +      (is_output && (par.pchan != s->channels)) ||
> +      (!is_output && (par.rchan != s->channels)) ||
> +      (par.rate != s->sample_rate)) {

Indentation is off.

  if (par.bits != 16 || par.sig != 1 || par.le != SIO_LE_NATIVE ||
      (is_output  && (par.pchan != s->channels)) ||
      (!is_output && (par.rchan != s->channels)) ||
      (par.rate != s->sample_rate)) {

or

  if (par.bits != 16 || par.sig != 1 || par.le != SIO_LE_NATIVE
      || (is_output  && (par.pchan != s->channels))
      || (!is_output && (par.rchan != s->channels))
      || (par.rate != s->sample_rate)) {

> +    s->buffer_size = par.round * par.bps *
> +      (is_output ? par.pchan : par.rchan);

ditto

  s->buffer_size = par.round * par.bps *
                   (is_output ? par.pchan : par.rchan);

> +    s->sample_rate = par.rate;
> +    s->bps = par.bps;

Align the =.

more below

> +    if (ret < 0) {
> +        return AVERROR(EIO);
> +    } else {
> +        return 0;
> +    }

superfluous {}

more below

> +static int audio_read_close(AVFormatContext *s1)

I think the open and close functions can be marked as av_cold.

> +#if CONFIG_SNDIO_INDEV
> +AVInputFormat sndio_demuxer = {
> +};
> +#endif
> +
> +#if CONFIG_SNDIO_OUTDEV
> +AVOutputFormat sndio_muxer = {

The #ifdefs could be avoided if muxer and demuxer were placed in
separate files.

> +#if HAVE_BIGENDIAN
> +    CODEC_ID_PCM_S16BE,
> +#else
> +    CODEC_ID_PCM_S16LE,
> +#endif

I faintly remember we had a machine/native endian version of this.

Diego