[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