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

[compn]

On Mon, 26 Sep 2011 20:37:19 +0200, Reimar Döffinger wrote:
>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.

cc'd to patch author. spdif users would like to have this.

-compn