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

[Naoya OYAMA]

Thanks for the review.
patch updated.

MPlayer have E-AC3 parser.
so, add E-AC3 entry in codecs.conf.
But E-AC3 pass through is not tested.
I don't have the E-AC3 file.

2011/8/12, Diego Biurrun <diego at biurrun.de>:
> 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?
Remove not needed #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.
Remove typedef.

>> +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.
I am sorry, I do not understand this sentence...
Is it good in this?

>> +static int preinit(sh_audio_t *sh) {
>
> Please use K&R style, i.e. place the { on the next line for function
> declarations.
Fix it.

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

>> +    ctx2 = av_mallocz(sizeof(spdifContext2));
>> +    if (ctx2 == NULL)
>
> if (!ctx2)
>
> more below
Fix it.

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

>> +    if(lavf_ctx->streams[0]->codec->codec_id == CODEC_ID_MP3){
>
> Please consistently format your if statements.
I am sorry, I do not understand this sentence...
Is it good in this?

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

>> +    fail:
>> +    mp_msg(MSGT_DECAUDIO,MSGL_INFO,"fail.\n");
>
> That's hardly informative, so you might as well drop it.
Remove it.

>> +static int decode_audio(sh_audio_t *sh, unsigned char *buf, int minlen,
>> int maxlen) {
>
> long line
Fix it.

>> +    spdif_ctx->out_buffer_len = 0;
>> +    spdif_ctx->out_buffer_size = maxlen;
>> +    spdif_ctx->out_buffer = buf;
>
> align the '='
Fix it.

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

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

>> +    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.
Remove the inner NULL checks.

commented-out code has pointer bug.
so, I fix problem. see below.

>> +            av_freep(lavf_ctx->pb);
>> +            //avio_close(lavf_ctx->pb);
av_freep(&lavf_ctx->pb)
avio_close is unnecessary.

>> +            //av_freep(&lavf_ctx->streams[0]->codec);
init()
+    AVStream             **streams  = NULL;
+    streams = av_mallocz(sizeof(AVStream*)*1);
+    if (!streams)
+        goto fail;
-    lavf_ctx->streams = &stream;
+    lavf_ctx->streams           = streams;
+    lavf_ctx->streams[0]        = stream;

>> +            //av_freep(&spdif_ctx->pb_buffer);
av_freep(lavf_ctx->pb) crash pointer,
so, av_freep(&spdif_ctx->pb_buffer) will send SIG_ABORT.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mplayer_spdif2.patch
Type: text/x-patch
Size: 17168 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20110814/08d273ce/attachment-0001.bin>