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

[Naoya OYAMA]

Hi,
Thank you for review.
Patch updated.

2011/9/27, Reimar Döffinger <Reimar.Doeffinger at gmx.de>:
> 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.
I think so.

>> @@ -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.
iec61937 move to be next to ac3.

>> @@ -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...
It modify to "AC3/iec61937/iec958".

>> +#define OUTBUF_SIZE (65536)
>
> Useless ()
Fix it.

>> + unsigned char *out_buffer;
>> + unsigned char *pb_buffer;
>
> uint8_t is the preferred type for that really.
Use uint8_t.

>> +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.
Fix it.

>> +static int64_t seek(void *p, int64_t offset, int whence)
>> +{
>> + //do nothing.
>> + return (int64_t)0LL;
>
> return 0;
> does exactly the same.
Fix it.

>> + 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.
Fix it.
spdifContext and spdifContext2 were combined.

>> + stream = av_mallocz(sizeof(AVStream));
>> + streams = av_mallocz(sizeof(AVStream*)*1);
>
> The same, though I think the lavc API forbids using sizeof on AVStream.
Fix it.
Useing av_new_stream().

>> + spdif_ctx->pb_buffer = av_mallocz(sizeof(unsigned char)*OUTBUF_SIZE);
>
> sizeof(char) is guaranteed to be 1...
Fix it.
pb_buffer had changed.
I think it should not be separately allocated.
+    uint8_t          pb_buffer[OUTBUF_SIZE];

>> + codec = av_mallocz(sizeof(AVCodecContext));
>
> same as for AVStream.
Fix it.

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

>> + ctx2 = (struct spdifContext2*)sh->context;
>
> Unnecessary cast.
Fix it.

>> + while ((spdif_ctx->out_buffer_len + spdif_ctx->iec61937_packet_size <
>> maxlen)
>> + && (spdif_ctx->out_buffer_len < minlen)) {
>
> The innermost () are unnecessary really
Fix it.

>> + 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.
Fix it.

>> + 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.
cannot be merged, moved in below.
if (lavf_ctx) {

Thank you.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mplayer_spdif4.patch
Type: text/x-patch
Size: 19356 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20111010/206d12bc/attachment-0001.bin>