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

[Reimar Döffinger]

On Fri, Sep 02, 2011 at 01:53:10AM +0900, Naoya OYAMA wrote:
> +/* set up max. outburst. use 131072 for TrueHD SPDIF psss-through,

"pass" not "psss" I guess.

> @@ -55,6 +55,9 @@ int af_str2fmt(const char* str)
>    if(strstr(str,"imaadpcm") || strstr(str,"IMAADPCM")){
>      format |= AF_FORMAT_IMA_ADPCM; return format;
>    }
> +  if(strstr(str,"iec61937") || strstr(str,"IEC61937")){
> +    format |= AF_FORMAT_IEC61937 | AF_FORMAT_16BIT; return format;
> +  }
>  
>    // Scan for int/float
>    if(strstr(str,"float") || strstr(str,"FLOAT")){
> @@ -123,6 +126,8 @@ char* af_fmt2str(int format, char* str, int size)
>        i+=snprintf(&str[i],size-i,"AC3 "); break;
>      case(AF_FORMAT_IMA_ADPCM):
>        i+=snprintf(&str[i],size-i,"IMA-ADPCM "); break;
> +    case(AF_FORMAT_IEC61937):
> +      i+=snprintf(&str[i],size-i,"IEC61937 "); break;
>      default:
>        i+=snprintf(&str[i],size-i,MSGTR_AF_FORMAT_UnknownFormat);
>      }
> @@ -161,6 +166,9 @@ static struct {
>      { "ac3le", AF_FORMAT_AC3_LE },
>      { "ac3be", AF_FORMAT_AC3_BE },
>      { "ac3ne", AF_FORMAT_AC3_NE },
> +    { "iec61937le", AF_FORMAT_IEC61937_LE },
> +    { "iec61937be", AF_FORMAT_IEC61937_BE },
> +    { "iec61937ne", AF_FORMAT_IEC61937_NE },
>      { "imaadpcm", AF_FORMAT_IMA_ADPCM },

That ordering is inconsistent.
I would prefer it to be next to ac3, even if it breaks alphabetical
order.

> @@ -418,9 +420,9 @@ static int init(int rate_hz, int channels, int format, int flags)
>       * while opening the abstract alias for the spdif subdevice
>       * 'iec958'
>       */
> -    if (AF_FORMAT_IS_AC3(format)) {
> +    if (AF_FORMAT_IS_AC3(format) || AF_FORMAT_IS_IEC61937(format)) {
>  	device.str = "iec958";
> -	mp_msg(MSGT_AO,MSGL_V,"alsa-spdif-init: playing AC3, %i channels\n", channels);
> +	mp_msg(MSGT_AO,MSGL_V,"alsa-spdif-init: playing IEC 61937, %i channels\n", channels);

Not that it matters much, but that is quite confusing, is it now AC3,
iec958 or IEC 61937...

> +#define OUTBUF_SIZE (65536)

Useless ()

> +    unsigned char *out_buffer;
> +    unsigned char *pb_buffer;

uint8_t is the preferred type for that really.

> +static int write_packet(void *p, uint8_t *buf, int buf_size)
> +{
> +    int len;
> +    struct spdifContext *ctx;
> +
> +    ctx = (struct spdifContext *)p;
> +    len = FFMIN(buf_size, ctx->out_buffer_size -ctx->out_buffer_len);
> +    memcpy(&ctx->out_buffer[ctx->out_buffer_len], buf, len);

The cast is unnecessary and the declarations and assignments can be
merged.

> +static int64_t seek(void *p, int64_t offset, int whence)
> +{
> +    //do nothing.
> +    return (int64_t)0LL;

return 0;
does exactly the same.

> +    ctx2 = av_mallocz(sizeof(struct spdifContext2));
> +    ctx2->spdif_ctx = av_mallocz(sizeof(struct spdifContext));

Please use sizeof(*ctx) / sizeof(*ctx2->spdif_ctx)
I wonder why two, separately allocated, contexts are necessary anyway,
I think it shouldn't be.

> +    stream = av_mallocz(sizeof(AVStream));
> +    streams = av_mallocz(sizeof(AVStream*)*1);

The same, though I think the lavc API forbids using sizeof on AVStream.

> +    spdif_ctx->pb_buffer = av_mallocz(sizeof(unsigned char)*OUTBUF_SIZE);

sizeof(char) is guaranteed to be 1...

> +    codec = av_mallocz(sizeof(AVCodecContext));

same as for AVStream.

> +    } else {
> +        if (sh->avctx) {

This should be "} else if (sh->avctx) {" without the extra unnecessary
block.

> +    ctx2 = (struct spdifContext2*)sh->context;

Unnecessary cast.

> +    while ((spdif_ctx->out_buffer_len + spdif_ctx->iec61937_packet_size < maxlen)
> +           && (spdif_ctx->out_buffer_len < minlen)) {

The innermost () are unnecessary really

> +        x = ds_get_packet_pts(sh->ds, &start, &pts);
> +        if (sh->ds->eof)
> +            break;

I think that's wrong when using the parser, you should see if
there's any data still stuck in the parser.

> +    if (lavf_ctx)
> +        if (lavf_ctx->oformat)

Can be merged in to a single if, or

> +            lavf_ctx->oformat->write_trailer(lavf_ctx);
> +
> +    if (lavf_ctx) {

Just moved in here.