[FFmpeg-devel] [PATCH]Add Dirac support to ffmpeg via libdirac_* and Schroedinger libraries]

Michael Niedermayer michaelni
Sat Apr 5 01:44:31 CEST 2008


On Fri, Apr 04, 2008 at 02:39:25PM +1100, Anuradha Suraparaju wrote:
> Sorry. Forgot to mention that the message is a patch in my earlier
> email.
> 
> Regards,
> Anuradha
> -------- Forwarded Message --------
> From: Anuradha Suraparaju <anuradha at rd.bbc.co.uk>
> Reply-To: FFmpeg development discussions and patches
> <ffmpeg-devel at mplayerhq.hu>
> To: ffmpeg-devel at mplayerhq.hu
> Cc: dirac at rd.bbc.co.uk, anuradha at rd.bbc.co.uk
> Subject: [FFmpeg-devel] Add Dirac support to ffmpeg via libdirac_* and
> Schroedinger	libraries
> Date: Fri, 04 Apr 2008 13:22:53 +1100
> 
> Hi,
> 
> I have attached a patch to FFmpeg to enable Dirac compression system
> support via the libdirac_encoder/decoder libraries and Schroedinger
> libraries. The attached README has instructions as to how to apply the
> patch and enable Dirac support.
[...]
> diff --exclude=.svn -ruN ffmpegsvn_trunk/libavcodec/avcodec.h ffmpegsvn_trunk_dirac_schro/libavcodec/avcodec.h
> --- ffmpegsvn_trunk/libavcodec/avcodec.h	2008-04-01 09:14:03.000000000 +1000
> +++ ffmpegsvn_trunk_dirac_schro/libavcodec/avcodec.h	2008-04-01 09:15:43.000000000 +1000
> @@ -182,6 +182,8 @@
>      CODEC_ID_8SVX_EXP,
>      CODEC_ID_8SVX_FIB,
>      CODEC_ID_ESCAPE124,
> +    CODEC_ID_SCHRO = 0xFFFE,
> +    CODEC_ID_DIRAC = 0xFFFF,

There should only be 1 codec_id and it should not have such a funny number.
Also spliting dirac/schroedinger/common demuxer/parser/decoder/encoder in
seperate patches would simplify review and likely reduce the number of
review-patch-resend iteration needed to get the stuff in svn.


[...]
> +/* ffmpeg qscale to Dirac quality conversion table */
> +static const float quality_factor_conv[] = {10.0, 9.8, 9.6, 9.4, 9.2, 9.0, 8.8, 
> +          8.6, 8.4, 8.2, 8.0, 7.8, 7.6, 7.3, 7.0, 6.7, 6.4, 6.0, 5.6, 5.2, 4.8, 
> +          4.4, 4.0, 3.6, 3.2, 2.8, 2.4, 2.0, 1.6, 1.2, 1.0 };

Using integers (100,98,96,... would ensure that the code behaves identical
on different architectures.

Also all comments about "stuff" should be doxygen compatible. /** ... */


> +
> +/* contains a single frame returned from Dirac*/
> +typedef struct FfmpegDiracOutputFrame
> +{
> +    /* frame data */
> +    unsigned char *p_data;

> +    

Trailing whitespace is forbidden in ffmpeg svn.


[...]
> +static int dirac_encode_init(AVCodecContext *avccontext)
> +{
> +  
> +    FfmpegDiracParams* p_dirac_params = avccontext->priv_data;
> +    int no_local=1;  
> +    int verbose=avccontext->debug;
> +    dirac_encoder_presets_t preset;
> +   
> +    /* get dirac preset*/
> +    preset = GetDiracPreset(avccontext);
> +

> +    /* set data to zero */
> +    memset (p_dirac_params, 0, sizeof(FfmpegDiracParams));

should be unneccesary


> + 
> +    /* initialise the encoder context */
> +    dirac_encoder_context_init (&(p_dirac_params->enc_ctx), preset);
> +   
> +    if (GetDiracChromaFormat(avccontext) == -1)
> +        return -1;

> +    p_dirac_params->enc_ctx.src_params.frame_rate.numerator=avccontext->time_base.den;
> +    p_dirac_params->enc_ctx.src_params.frame_rate.denominator=avccontext->time_base.num;
> +    p_dirac_params->enc_ctx.src_params.width=avccontext->width;
> +    p_dirac_params->enc_ctx.src_params.height=avccontext->height;

could be vertically aligned like:

 p_dirac_params->enc_ctx.src_params.frame_rate.numerator  =avccontext->time_base.den;
 p_dirac_params->enc_ctx.src_params.frame_rate.denominator=avccontext->time_base.num;
 p_dirac_params->enc_ctx.src_params.width =avccontext->width;
 p_dirac_params->enc_ctx.src_params.height=avccontext->height;



[...]

> +    /* allocate enough memory for the incoming data */
> +    p_dirac_params->p_in_frame_buf = (unsigned char*) av_malloc(p_dirac_params->frame_size);

The cast is unneeded.


[...]
> +static int dirac_encode_frame(AVCodecContext *avccontext,
> +                              unsigned char *frame,
> +                              int buf_size, void *data)
> +{
> +    int enc_size=0;
> +    dirac_encoder_state_t state;
> +    FfmpegDiracParams* p_dirac_params = avccontext->priv_data;
> +    struct FfmpegDiracOutputFrame* p_frame_output=NULL;
> +    struct FfmpegDiracOutputFrame* p_next_output_frame=NULL;
> +    int go = 1;
> +

> +    if (data==0) {

I think data==NULL or !data is clearer, after all this is a pointer not
an interger like 0.


[...]
> +            p_frame_output=(struct FfmpegDiracOutputFrame*)av_malloc(sizeof(FfmpegDiracOutputFrame));
> +            memset(p_frame_output, 0, sizeof(FfmpegDiracOutputFrame));

The cast is unneeded and av_mallocz() does the memset as well.


> +                
> +            /* set output data */
> +            p_frame_output->p_data=(unsigned char*)av_malloc(p_dirac_params->p_encoder->enc_buf.size);
> +            memcpy(p_frame_output->p_data,p_dirac_params->p_encoder->enc_buf.buffer,p_dirac_params->p_encoder->enc_buf.size);
> +            p_frame_output->size=p_dirac_params->p_encoder->enc_buf.size;
> +            p_frame_output->type=p_dirac_params->p_encoder->enc_pparams.ptype;

> +            if (p_dirac_params->p_next_output_frame==NULL) {
> +                p_dirac_params->p_next_output_frame=p_frame_output;
> +                p_dirac_params->p_last_output_frame=p_frame_output;
> +            } else {
> +                p_dirac_params->p_last_output_frame->p_next_frame=p_frame_output;
> +                p_dirac_params->p_last_output_frame=p_frame_output;
> +            }

"p_dirac_params->p_last_output_frame=p_frame_output;" can be factored out


> +            if (state == ENC_STATE_EOS) {
> +                p_dirac_params->eos_pulled = 1;
> +                go = 0;
> +            }
> +            break;
> +
> +        case ENC_STATE_BUFFER:
> +            go = 0;
> +            break;
> +
> +        case ENC_STATE_INVALID:
> +            av_log(avccontext, AV_LOG_ERROR, "Unrecoverable Encoder Error. Quitting...\n");
> +            return -1;
> +                
> +        default:
> +            av_log(avccontext, AV_LOG_ERROR, "Unknown Encoder state\n");
> +            return -1;
> +        }
> +    }
> +
> +    /* copy 'next' frame in queue */
> +    p_next_output_frame=p_dirac_params->p_next_output_frame;
> +    if (p_next_output_frame==NULL)
> +        return 0;
> +
> +    memcpy(frame, p_next_output_frame->p_data, p_next_output_frame->size);
> +    avccontext->coded_frame->key_frame= p_next_output_frame->type == INTRA_PICTURE;    

> +    avccontext->coded_frame->pts= AV_NOPTS_VALUE;    

This should be set to the correct pts from the matching input AVFrame.


[...]

> +static int dirac_encode_close(AVCodecContext *avccontext)
> +{
> +    FfmpegDiracParams* p_dirac_params = avccontext->priv_data;
> +        
> +     /* close the encoder */
> +    dirac_encoder_close(p_dirac_params->p_encoder );     
> +
> +    av_freep(&p_dirac_params->p_in_frame_buf);
> +    
> +    return 0 ;
> +}

Either theres a possible memleak with p_*_output_frame or the linked list
is unneeded or i misunderstand the code.


> +
> +
> +AVCodec dirac_encoder = {
> +    "dirac",
> +    CODEC_TYPE_VIDEO,
> +    CODEC_ID_DIRAC,
> +    sizeof(FfmpegDiracParams),
> +    dirac_encode_init,
> +    dirac_encode_frame,
> +    dirac_encode_close,
> +   .capabilities= CODEC_CAP_DELAY,
> +} ;
> +
> +/*-------------------------DECODER------------------------------------------*/

The decoder should be in a seperate file so users who do not want any encoders
can compile just the decoder.


> +
> +static int dirac_decode_init(AVCodecContext *avccontext)
> +{
> +
> +    FfmpegDiracParams *p_dirac_params = (FfmpegDiracParams*)avccontext->priv_data ;

unneeded cast.


> +    p_dirac_params->p_decoder =  dirac_decoder_init(avccontext->debug);
> +
> +    if (!p_dirac_params->p_decoder)
> +        return -1;
> +
> +    return 0 ;
> +}
> +
> +static int dirac_decode_frame(AVCodecContext *avccontext,
> +                              void *data, int *data_size,
> +                              uint8_t *buf, int buf_size)
> +{

> +   
> +    FfmpegDiracParams *p_dirac_params=(FfmpegDiracParams*)avccontext->priv_data;
> +    AVPicture *picture = (AVPicture*)data;

unneeded casts
(these are of course minor points if you prefer the C++ look ...)


> +    AVPicture pic;
> +    int pict_size;
> +    unsigned char *buffer[3];
> +
> +    *data_size = 0;
> +
> +    if (buf_size>0) 
> +        /* set data to decode into buffer */
> +        dirac_buffer (p_dirac_params->p_decoder, buf, buf+buf_size);
> +
> +    while (1) {
> +         /* parse data and process result */
> +        DecoderState state = dirac_parse (p_dirac_params->p_decoder);
> +        switch (state)
> +        {
> +        case STATE_BUFFER:
> +            return buf_size;
> +
> +        case STATE_SEQUENCE:
> +            /* tell ffmpeg about sequence details*/

> +            avccontext->height=p_dirac_params->p_decoder->src_params.height;
> +            avccontext->width=p_dirac_params->p_decoder->src_params.width;

vertical align


> +            avccontext->pix_fmt=GetFfmpegChromaFormat(p_dirac_params->p_decoder->src_params.chroma);
> +            avccontext->time_base.den =p_dirac_params->p_decoder->src_params.frame_rate.numerator;
> +            avccontext->time_base.num =p_dirac_params->p_decoder->src_params.frame_rate.denominator;
> +        
> +            /* calc output dimensions */
> +            avpicture_fill(&pic, NULL, avccontext->pix_fmt, avccontext->width, avccontext->height); 
> +            pict_size = avpicture_get_size(avccontext->pix_fmt, avccontext->width, avccontext->height);
> +            
> +            /* allocate output buffer */
> +            if (p_dirac_params->p_out_frame_buf==0)
> +                p_dirac_params->p_out_frame_buf = (unsigned char *)av_malloc (pict_size);

> +            buffer[0]=p_dirac_params->p_out_frame_buf;
> +            buffer[1]=p_dirac_params->p_out_frame_buf+(pic.linesize[0]*avccontext->height);
> +            buffer[2]=buffer[1]+(pic.linesize[1]*p_dirac_params->p_decoder->src_params.chroma_height);

This would be more readable with a few spaces IMHO, like:

buffer[2]= buffer[1] + pic.linesize[1] * p_dirac_params->p_decoder->src_params.chroma_height;


> +                
> +            /* tell dirac about output destination */
> +            dirac_set_buf(p_dirac_params->p_decoder, buffer, NULL);
> +            break;
> +                        
> +        case STATE_SEQUENCE_END:
> +            break;
> +
> +        case STATE_PICTURE_AVAIL:
> +            /* fill pic with current buffer data from dirac*/
> +            avpicture_fill(picture, p_dirac_params->p_out_frame_buf, avccontext->pix_fmt, avccontext->width, avccontext->height);

> +            *data_size=1;

Should be sizeof(AVFrame) or sizeof(AVPicture)


[...]
> diff --exclude=.svn -ruN ffmpegsvn_trunk/libavcodec/dirac_parser.c ffmpegsvn_trunk_dirac_schro/libavcodec/dirac_parser.c
> --- ffmpegsvn_trunk/libavcodec/dirac_parser.c	1970-01-01 10:00:00.000000000 +1000
> +++ ffmpegsvn_trunk_dirac_schro/libavcodec/dirac_parser.c	2008-04-04 11:28:33.000000000 +1000
[...]
ffmpegsvn_trunk_dirac_schro/libavcodec/dirac_schro_parser.c
> --- ffmpegsvn_trunk/libavcodec/dirac_schro_parser.c	1970-01-01 10:00:00.000000000 +1000
> +++ ffmpegsvn_trunk_dirac_schro/libavcodec/dirac_schro_parser.c	2008-04-04 11:28:45.000000000 +1000
[...]
> diff --exclude=.svn -ruN ffmpegsvn_trunk/libavcodec/dirac_schro_parser.h ffmpegsvn_trunk_dirac_schro/libavcodec/dirac_schro_parser.h
> --- ffmpegsvn_trunk/libavcodec/dirac_schro_parser.h	1970-01-01 10:00:00.000000000 +1000
> +++ ffmpegsvn_trunk_dirac_schro/libavcodec/dirac_schro_parser.h	2008-04-04 11:29:23.000000000 +1000

Not reviewed because i hope that you can use the gsoc dirac parser.
Which does look quite a bit simpler.


[...]

> diff --exclude=.svn -ruN ffmpegsvn_trunk/libavcodec/schroedinger.c ffmpegsvn_trunk_dirac_schro/libavcodec/schroedinger.c
> --- ffmpegsvn_trunk/libavcodec/schroedinger.c	1970-01-01 10:00:00.000000000 +1000
> +++ ffmpegsvn_trunk_dirac_schro/libavcodec/schroedinger.c	2008-04-04 11:29:00.000000000 +1000
[...]
> +/* contains a single encoded frame returned from Schroedinger */
> +typedef struct FfmpegSchroOutputEncFrame
> +{
> +    /* frame data */
> +    unsigned char *p_data;
> +    
> +    /* frame size */
> +    int size;
> +    
> +    /* key frame flag */
> +    int key_frame;
> +    
> +    /* next frame to be output in sequence */
> +    struct FfmpegSchroOutputEncFrame *p_next_frame;
> +    
> + } FfmpegSchroOutputEncFrame;

code duplication relative to dirac


[...]
> +/*
> +* Works out Schro-compatible pre-set option from file size
> +*/
> +static SchroVideoFormatEnum GetSchroVideoFormatPreset(AVCodecContext *avccontext)
> +{
> +    if(avccontext->width==176 && avccontext->height==120)
> +        return SCHRO_VIDEO_FORMAT_QSIF;
> +    else if(avccontext->width==176 && avccontext->height==144)
> +        return SCHRO_VIDEO_FORMAT_QCIF;
> +    else if(avccontext->width==352 && avccontext->height==240)
> +        return SCHRO_VIDEO_FORMAT_SIF;
> +    else if(avccontext->width==352 && avccontext->height==288)
> +        return SCHRO_VIDEO_FORMAT_CIF;
> +    else if(avccontext->width==704 && avccontext->height==480)
> +        return SCHRO_VIDEO_FORMAT_4SIF;
> +    else if(avccontext->width==704 && avccontext->height==576)
> +        return SCHRO_VIDEO_FORMAT_4CIF;
> +    else if(avccontext->width==720 && avccontext->height==480)
> +        return SCHRO_VIDEO_FORMAT_SD480I_60;
> +    else if(avccontext->width==720 && avccontext->height==576)
> +        return SCHRO_VIDEO_FORMAT_SD576I_50;
> +    else if(avccontext->height==1280 && avccontext->height==720) {
> +        if (avccontext->time_base.den == 60000 && 
> +            avccontext->time_base.num == 1001) {
> +            return SCHRO_VIDEO_FORMAT_HD720P_60;
> +        } else {
> +            return SCHRO_VIDEO_FORMAT_HD720P_50;
> +        }
> +    } else if (avccontext->height==1920 && avccontext->width==1080) {
> +        if (avccontext->time_base.den == 30000 && 
> +            avccontext->time_base.num == 1001) {
> +            return SCHRO_VIDEO_FORMAT_HD1080I_60;
> +        } else if (avccontext->time_base.den == 25 && 
> +            avccontext->time_base.num == 1) {
> +            return SCHRO_VIDEO_FORMAT_HD1080I_50;
> +        } else if (avccontext->time_base.den == 60 && 
> +            avccontext->time_base.num == 1001) {
> +            return SCHRO_VIDEO_FORMAT_HD1080P_60;
> +        } else {
> +            return SCHRO_VIDEO_FORMAT_HD1080P_50;
> +        }
> +    } else if(avccontext->height==2048 && avccontext->width==1080)
> +        return SCHRO_VIDEO_FORMAT_DC2K_24;
> +    else if(avccontext->height==4096 && avccontext->width==2160)
> +        return SCHRO_VIDEO_FORMAT_DC4K_24;
> +    return SCHRO_VIDEO_FORMAT_CUSTOM;
> +}
> +
> +/*
> +* Works out Schro-compatible chroma format 
> +*/
> +static int GetSchroChromaFormat(AVCodecContext *avccontext)
> +{
> +    FfmpegSchroParams* p_schro_params = avccontext->priv_data;
> + 
> +    switch(avccontext->pix_fmt)
> +    {
> +    case PIX_FMT_YUV420P:
> +        p_schro_params->format->chroma_format =  SCHRO_CHROMA_420;
> +        break;
> +
> +    case PIX_FMT_YUV422P:
> +        p_schro_params->format->chroma_format =  SCHRO_CHROMA_422;
> +        break;
> +    
> +    case PIX_FMT_YUV444P:
> +        p_schro_params->format->chroma_format =  SCHRO_CHROMA_444;
> +        break;
> +    
> +    default:
> +        av_log (avccontext, AV_LOG_ERROR, "this codec supports only Planar YUV 4:2:0, 4:2:2 and 4:4:4 formats currently\n");
> +        return -1;
> +    }
> +    return 0;
> +}
> + 
> + /*
> + * returns Ffmppeg chroma format 
> + */
> +static int GetFfmpegChromaFormat(SchroChromaFormat schro_chroma)
> +{
> +    switch(schro_chroma)
> +    {
> +    case SCHRO_CHROMA_420:
> +        return PIX_FMT_YUV420P;
> +    case SCHRO_CHROMA_422:
> +        return PIX_FMT_YUV422P;
> +    case SCHRO_CHROMA_444:
> +        return PIX_FMT_YUV444P;
> +    
> +    default:
> +        break;
> +    }
> +      
> +     return PIX_FMT_YUV420P;
> +}

That looks quite similar to dirac
Also theres no need for a forward and backward switch-case.
A simple table of the ffmpeg+dirac+shchro IDs could be used.
Similarly for width-heigt-...


[...]
> diff --exclude=.svn -ruN ffmpegsvn_trunk/libavformat/raw.c ffmpegsvn_trunk_dirac_schro/libavformat/raw.c
> --- ffmpegsvn_trunk/libavformat/raw.c	2008-01-16 11:31:50.000000000 +1100
> +++ ffmpegsvn_trunk_dirac_schro/libavformat/raw.c	2008-04-01 13:45:44.000000000 +1000

Not reviewed as i hope that the gsoc demuxer can be used. Its in 
soc/dirac/ffmpeg.diff

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080405/5dd322b2/attachment.pgp>



More information about the ffmpeg-devel mailing list