[Ffmpeg-devel] [PATCH] dirac 0.6 support.
Michael Niedermayer
michaelni
Sun Feb 18 23:18:26 CET 2007
Hi
On Sat, Feb 17, 2007 at 02:59:47AM +0300, Pavlov Konstantin wrote:
> Hi guys. The attached file contains dirac-0.6 support to ffmpeg-7881.
> It's a bit outdated, but anyway is better than the one provided by dirac
> authors themselves.
besides what mans already said ...
[...]
> +/** contains a single frame returned from Dirac*/
> +typedef struct FfmpegDiracOutputFrame
> +{
> + /** frame data */
> + unsigned char *p_data;
> +
> + /** frame size */
> + int size;
> +
> + /** frame type */
> + int type;
> +
> + /** next frame to be output in sequence */
> + struct FfmpegDiracOutputFrame *p_next_frame;
> +
> + } FfmpegDiracOutputFrame;
> +
> +typedef struct FfmpegDiracParams
> +{
> + /* context params */
> + dirac_encoder_context_t enc_ctx;
> +
> + /* frame being encoded */
> + AVFrame picture;
> +
> + /* decoder */
> + dirac_decoder_t* p_decoder;
> +
> + /* encoder */
> + dirac_encoder_t* p_encoder;
> +
> + /* input frame buffer */
> + unsigned char *p_in_frame_buf;
> +
> + /** output frame buf */
> + unsigned char* p_out_frame_buf;
> +
> + /** next frame to be output*/
> + struct FfmpegDiracOutputFrame *p_next_output_frame;
> +
> + /** last frame to be output*/
> + struct FfmpegDiracOutputFrame *p_last_output_frame;
> +
> +} FfmpegDiracParams;
almost all the comments are useless, a *frame structure with a field named
size doesnt benefit from a comment saying that its the frame size
also comments would be more readable if they where at the right of the fields
instead of above them
> +
> +
> +typedef struct FfmpegDiracParseContext
> +{
> +
> + /** stream buffer variables */
> + unsigned char* p_dirac_videobuffer;
> + int dirac_videobuf_code_len;
doxygen comments applying to several fields must use the appropriate syntax
no i dont remember exactly it was something like ///@{ ///@} or so i think
[...]
> +/**
> +* Works out Dirac-compatible pre-set option from file size
> +*/
> +static dirac_encoder_presets_t GetDiracPreset(AVCodecContext *avccontext)
> +{
> +
> + if(avccontext->width==720 && avccontext->height==576)
> + return VIDEO_FORMAT_SD_625_DIGITAL;
> +
> + if(avccontext->height==1280 && avccontext->height==720)
> + return VIDEO_FORMAT_HD_720;
> +
> + if(avccontext->height==1920 && avccontext->width==1080)
> + return VIDEO_FORMAT_HD_1080;
> +
> + if(avccontext->height==352 && avccontext->width==288)
> + return VIDEO_FORMAT_CIF;
this could be written slighly nicer with a table + for loop
[...]
> +/**
> +* Works out Dirac-compatible chroma format
> +*/
> +static int GetDiracChromaFormat(AVCodecContext *avccontext)
> +{
> + FfmpegDiracParams* p_dirac_params = avccontext->priv_data;
> +
> + switch(avccontext->pix_fmt)
> + {
> + case PIX_FMT_YUV420P:
> + p_dirac_params->enc_ctx.seq_params.chroma = format420;
> + break;
> +
> + case PIX_FMT_YUV422P:
> + p_dirac_params->enc_ctx.seq_params.chroma = format422;
> + break;
> +
> + case PIX_FMT_YUV444P:
> + p_dirac_params->enc_ctx.seq_params.chroma = format444;
> + break;
> +
> + default:
> + av_log (avccontext, AV_LOG_ERROR, "this codec supports only Planar YUV formats (yuv420p, yuv422p, yuv444p\n");
> + return -1;
> + }
as above a table + for loop would be nicer
> + return format420;
this is wrong
> +}
> +
> + /**
> + * returns Ffmppeg chroma format
> + */
> +static int GetFfmpegChromaFormat(dirac_chroma_t dirac_chroma)
> +{
> + switch(dirac_chroma)
> + {
> + case format420:
> + return PIX_FMT_YUV420P;
> + case format422:
> + return PIX_FMT_YUV422P;
> + case format444:
> + return PIX_FMT_YUV444P;
> +
> + default:
> + break;
> + }
duplicates the format mapping (should use the same table as the function
above)
> +
> + return PIX_FMT_YUV420P;
this too is wrong
[...]
> + if (no_local)
> + {
> + p_dirac_params->enc_ctx.decode_flag = 0;
> + p_dirac_params->enc_ctx.instr_flag = 0;
> + }
> + else
> + {
> + p_dirac_params->enc_ctx.decode_flag = 1;
> + p_dirac_params->enc_ctx.instr_flag = 1;
> + }
what is this code good for? no_local is always 0
> +
> + if(avccontext->global_quality!=0)
> + p_dirac_params->enc_ctx.enc_params.qf=quality_factor_conv[(avccontext->global_quality/FF_QP2LAMBDA)-1];
> +
> + p_dirac_params->p_encoder = dirac_encoder_init( &(p_dirac_params->enc_ctx), verbose );
> +
> +
> +
> + if (!p_dirac_params->p_encoder)
> + {
> + av_log(avccontext, AV_LOG_ERROR, "Unrecoverable Error: dirac_encoder_init failed. ");
> + return EXIT_FAILURE;
this is wrong whatever EXIT_FAILURE is its not a valid return for the init
function of a codec
[...]
> +static int dirac_encode_frame(AVCodecContext *avccontext,
> + unsigned char *frame,
> + int buf_size, void *data)
do not give variables random names, if theres a buf_size then the coresponding
buffer should be named buf
> +{
> + int enc_size=0;
> + dirac_encoder_state_t state;
> + FfmpegDiracParams* p_dirac_params = avccontext->priv_data;
please use a shorter variable name, this makes the code fairly hard to read
also the random use of p_ prefixes should be avoided, id say never
use them, but if there is some place where indicating that something is a
pointer helps readablity then by all means keep the p_ prefix, here it
doesnt help IMHO though
[...]
> + p_dirac_params->picture = *(AVFrame*)data;
> + p_frame_src=(AVFrame*)data;
> +
> + /** allocate frame data to dirac input buffer */
missplaced doxygen comment
> + /*
> + * input line size may differe from what the codec supports. Especially
> + * when transcoding from one format to another. So use avpicture_layout
> + * to copy the frame.
> + */
> + avpicture_layout ((AVPicture *)data, avccontext->pix_fmt, avccontext->width, avccontext->height,p_dirac_params->p_in_frame_buf, avccontext->frame_size);
> +
> + /* load next frame*/
> + if (dirac_encoder_load( p_dirac_params->p_encoder, p_dirac_params->p_in_frame_buf, avccontext->frame_size ) < 0)
> + {
> + av_log(avccontext, AV_LOG_ERROR, "Unrecoverable Encoder Error. Quitting...\n");
> + return -1;
> + }
> +
> +
> + do {
> + p_dirac_params->p_encoder->enc_buf.buffer = frame;
> + p_dirac_params->p_encoder->enc_buf.size = buf_size;
> + /* process frame */
> + state = dirac_encoder_output ( p_dirac_params->p_encoder );
why is the encoding in a do loop? this seems wrong
also the whole next/last buffer management looks very wrong what is this
doing, ive taken a quick look at the api the only thing you should do is
feed the input frame into the encoder with dirac_encoder_load() and
then call 1 dirac_encoder_output() and return its output if there is any
the encoder cannot output more frames then have been input if it does they
are not frames and that requires some elaboration from you on what exactly
is outputed by this function
> +
> + switch (state)
> + {
> + case ENC_STATE_AVAIL:
> + assert (p_dirac_params->p_encoder->enc_buf.size > 0);
> + /* create output frame*/
> + p_frame_output=(struct FfmpegDiracOutputFrame*)av_malloc(sizeof(FfmpegDiracOutputFrame));
senseless cast which makes the code hard to read (applies to all malloc
return casts)
> + memset(p_frame_output, 0, sizeof(FfmpegDiracOutputFrame));
av_mallocz()
[...]
> + avccontext->coded_frame->pts= 0;
this is invalid only 0 or 1 frames may have 0 as pts
[...]
> +/**-------------------------DECODER------------------------------------------*/
the decoder should be in a seperate file and patch
[...]
> +static int dirac_sync_video_packet (const uint8_t **buf, int buf_size,
> + FfmpegDiracParseContext *p_dirac_params,
> + int *start)
> +{
> + int bytes_read = 0;
> + unsigned char* p_dirac_videobuf_code = p_dirac_params->dirac_videobuf_code;
> + p_dirac_params->dirac_videobuf_len=0;
> +
> + while(p_dirac_params->dirac_videobuf_code_len<5)
> + {
> + p_dirac_videobuf_code[p_dirac_params->dirac_videobuf_code_len++]=*(*buf);
> + (*buf)++;
> + ++bytes_read;
> + }
> + while (1)
> + {
> + int c;
> + if(p_dirac_videobuf_code[0]==0x42 && p_dirac_videobuf_code[1]==0x42 && p_dirac_videobuf_code[2]==0x43 && p_dirac_videobuf_code[3]==0x44)
> + break;
> +
> + if (bytes_read >= buf_size )
> + return -1;
> +
> + ++(*start);
> + p_dirac_videobuf_code[0]=p_dirac_videobuf_code[1];
> + p_dirac_videobuf_code[1]=p_dirac_videobuf_code[2];
> + p_dirac_videobuf_code[2]=p_dirac_videobuf_code[3];
> + p_dirac_videobuf_code[3]=p_dirac_videobuf_code[4];
> + c = *(*buf);
> + (*buf)++;
> + ++bytes_read;
> + p_dirac_videobuf_code[4]=c;
> + }
> +
> + return p_dirac_videobuf_code[4];
this whole code really is a terrible mess
this is nothing else then:
uint32_t state=0;
for(i=0; i+1 < buf_size; i++){
state = (state<<8) + *(*buf)++;
if(state == 0x42424344)
return *(*buf)++;
return -1;
}
though your code is probably wrong to begin with as these codes can be
split between packets
> +}
> +
> +static int dirac_find_frame_end(FfmpegDiracParseContext *p_dirac_params,
> + const uint8_t *buf,
> + uint8_t *p_buf_end, int cur_idx)
> +{
> + int byte;
> + int end_idx=cur_idx;
> +
> + /* find end of frame */
> + int shift = 0xffffffff;
> + p_dirac_params->dirac_videobuf_code_len = 0;
> + while (p_dirac_params->in_frame)
> + {
> + if(buf==p_buf_end)
> + return -1;
> +
> + byte = *buf;
> + if (byte < 0)
> + return -1;
> +
> + if (shift == 0x42424344)
> + {
> + p_dirac_params->in_frame = 0;
> + p_dirac_params->dirac_videobuf_code_len = 5;
> + p_dirac_params->dirac_videobuf_code[0] = 0x42;
> + p_dirac_params->dirac_videobuf_code[1] = 0x42;
> + p_dirac_params->dirac_videobuf_code[2] = 0x43;
> + p_dirac_params->dirac_videobuf_code[3] = 0x44;
> + p_dirac_params->dirac_videobuf_code[4] = byte;
> + return end_idx;
> + }
> + shift = (shift << 8 ) | byte;
> + buf++;
> + end_idx++;
> + }
this partially dupliactes the function above
> +
> + return -1;
> +
> +}
> +
> +/*
> +* Access unit header = 0x00
> +* Intra_Ref start = 0x0C
> +* Intra_NonRef start = 0x08
> +* Inter_Ref_1Ref start = 0x0D
> +* Inter_Ref_2Ref start = 0x0E
> +* Inter_NonRef_1Ref start = 0x09
> +* Inter_NonRef_2Ref start = 0x0A
> +* End of sequence = 0x10
> +*/
> +#define FRAME_START(c) ((c) == 0x00 || (c) == 0x0C || (c) == 0x08 || (c) == 0x0D || (c) == 0x0E || (c) == 0x09 || (c) == 0x0A || (c) == 0x10)
> +
> +static int dirac_find_frame_start(FfmpegDiracParseContext *p_dirac_params, const uint8_t **buf, int buf_size)
> +{
> + unsigned int shift = 0xffffffff;
> + int msg_type = 0;
> + int start=0;
> +
> + /* find start of data */
> + msg_type = dirac_sync_video_packet(buf, buf_size, p_dirac_params, &start);
> +
> + if (msg_type < 0)
> + return -1;
> +
> +
> + /* find start of frame */
> + while (!p_dirac_params->in_frame)
> + {
> + int byte;
> + if (FRAME_START(msg_type))
> + {
> + p_dirac_params->in_frame = 1;
> + return start;
> + }
> +
> + byte = *(*buf);
> + (*buf)++;
> +
> + if (byte < 0)
as **buf is unsigned byte cannot be <0
your parser is very messy and not accpetable for ffmpeg svn please
clean it up
[...]
> +/* drc read */
> +static int dirac_read_header(AVFormatContext *s,
> + AVFormatParameters *ap)
> +{
> + AVStream *st;
> +
> + st = av_new_stream(s, 0);
> + if (!st)
> + return AVERROR_NOMEM;
> +
> + st->codec->codec_type = CODEC_TYPE_VIDEO;
> + st->codec->codec_id = CODEC_ID_DIRAC;
> + st->need_parsing = 1;
> + /* the parameters will be extracted from the compressed bitstream */
> + return 0;
> +}
> +
use video_read_header()
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The educated differ from the uneducated as much as the living from the
dead. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070218/d726616c/attachment.pgp>
More information about the ffmpeg-devel
mailing list