[MPlayer-dev-eng] [PATCH]SPDIF pass through decoder

[Diego Biurrun]

On Thu, Aug 11, 2011 at 11:38:42PM +0900, Naoya OYAMA wrote:
> I write SPDIF pass through decoder in MPlayer.
> This decoder use ffmpeg/libavformat/spdifenc.c .

.. cursory first round of review ..

> --- /dev/null
> +++ b/libmpcodecs/ad_spdif.c
> @@ -0,0 +1,311 @@
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include "config.h"
> +
> +#include "mp_msg.h"
> +#include "help_mp.h"
> +
> +#include "ad_internal.h"
> +#include "dec_audio.h"
> +#include "libavfilter/libmpcodecs/vd_ffmpeg.h"
> +#include "libaf/reorder_ch.h"
> +#include "fmt-conversion.h"
> +
> +#include "libavcodec/avcodec.h"
> +#include "libavformat/avformat.h"
> +#include "libavutil/avutil.h"
> +#include "libavutil/opt.h"

Do you need all these #includes?

> +typedef struct spdifContext {
> +    int            out_buffer_len;
> +    int            out_buffer_size;
> +    unsigned char *out_buffer;
> +    unsigned char *pb_buffer;
> +} spdifContext;
> +
> +typedef struct spdifContext2 {
> +    spdifContext    *spdif_ctx;
> +    AVFormatContext *lavf_ctx;
> +} spdifContext2;

I dislike typedefs for structs.

> +static int spdif_read_packet(void *p, uint8_t *buf, int buf_size);
> +static int spdif_write_packet(void *p, uint8_t *buf, int buf_size);
> +static int64_t spdif_seek(void *p, int64_t offset, int whence);

Reorder your functions to avoid forward declarations.

> +static int preinit(sh_audio_t *sh) {

Please use K&R style, i.e. place the { on the next line for function
declarations.

> +static int init(sh_audio_t *sh) {
> +    int i;
> +    int x;
> +    unsigned char *start;
> +    double pts;
> +    int in_size;

You could save a few lines by declaring the ints together.

more below

> +    ctx2 = av_mallocz(sizeof(spdifContext2));
> +    if (ctx2 == NULL)

if (!ctx2)

more below

> +    for(i=0; fmt_id_type[i].name; i++) {

for (i = 0; fmt_id_type[i].name; i++) {

> +    if(lavf_ctx->streams[0]->codec->codec_id == CODEC_ID_MP3){

Please consistently format your if statements.

> +        if (sh->avctx->sample_rate < 24000) {
> +            mp_msg(MSGT_DECAUDIO,MSGL_INFO,
> +                    "This stream sample_rate[%d Hz] may be broken. "
> +                    "Force reset 48000Hz.\n",
> +                    sh->avctx->sample_rate);

Indentation is off.

> +    fail:
> +    mp_msg(MSGT_DECAUDIO,MSGL_INFO,"fail.\n");

That's hardly informative, so you might as well drop it.

> +static int decode_audio(sh_audio_t *sh, unsigned char *buf, int minlen, int maxlen) {

long line

> +    spdif_ctx->out_buffer_len = 0;
> +    spdif_ctx->out_buffer_size = maxlen;
> +    spdif_ctx->out_buffer = buf;

align the '='

> +        mp_msg(MSGT_DECAUDIO,MSGL_V,"start[%x] pkt.size[%d] in_size[%d] consumed[%d] x[%d].\n",
> +                pkt.data, start, pkt.size, in_size, consumed, x);

Indentation is off.

> +static int control(sh_audio_t *sh, int cmd, void* arg, ...) {
> +    unsigned char *start;
> +    double pts;
> +
> +    switch(cmd){

switch (cmd) {

> +    if (lavf_ctx) {
> +        if (lavf_ctx->pb) {
> +            av_freep(lavf_ctx->pb);
> +            //avio_close(lavf_ctx->pb);
> +        }
> +        if (lavf_ctx->streams[0]) {
> +            //av_freep(&lavf_ctx->streams[0]->codec);
> +            av_freep(&lavf_ctx->streams[0]);
> +        }
> +        av_freep(&lavf_ctx->priv_data);
> +        av_freep(&lavf_ctx);
> +    }
> +    if (spdif_ctx) {
> +        if (spdif_ctx->pb_buffer) {
> +            //av_freep(&spdif_ctx->pb_buffer);
> +        }
> +        av_freep(&spdif_ctx);
> +    }
> +    if (ctx2)
> +        av_freep(&ctx2);
> +}

Why all the commented-out code?  The inner NULL checks are unnecessary.

Diego