[FFmpeg-devel] [PATCH 1/2] MxPEG decoder

[Anatoly Nenashev]

On 14.02.2011 14:51, M?ns Rullg?rd wrote:
> Anatoly Nenashev<anatoly.nenashev at ovsoft.ru>  writes:
>
>    
>>
>> +static int mjpeg_allocate_picture(MJpegDecodeContext *s, AVFrame* picture_ptr,
>> +                                  int key_frame, int reference_frame)
>>      
> Style nit: please keep stars attached to variable names, like this:
> AVFrame *picture_ptr
>
>    
>> +{
>> +    int i;
>> +
>> +    if (picture_ptr->data[0])
>> +        s->avctx->release_buffer(s->avctx, picture_ptr);
>> +
>> +    picture_ptr->reference = reference_frame;
>> +    if (s->avctx->get_buffer(s->avctx, picture_ptr)<  0) {
>> +        av_log(s->avctx, AV_LOG_ERROR, "get_buffer() failed\n");
>> +        return -1;
>> +    }
>> +    picture_ptr->pict_type = key_frame ? FF_I_TYPE : FF_P_TYPE;
>> +    picture_ptr->key_frame = key_frame;
>> +    s->got_picture = 1;
>> +
>> +    for (i = 0; i<  MAX_COMPONENTS; i++) {
>> +        s->linesize[i] = picture_ptr->linesize[i]<<  s->interlaced;
>> +    }
>> +}
>>      
> Is this purely factored out of the code below?  If so, separating such
> changes into a preparatory patch would make the main patch easier to
> read.  If it's not too much trouble...
>
>    

I will try to do it this way in new review thread which I've created.
>> +    reference_ptr =&mxctx->pictures[mxctx->picture_index ^ 1];
>> +    if (!reference_ptr->data[0]) {
>> +        av_log(s->avctx, AV_LOG_WARNING, "No reference picture data for non-key frame, skipping\n");
>> +        return -1;
>> +    }
>> +
>> +    picture_ptr =&mxctx->pictures[mxctx->picture_index];
>> +    if (mjpeg_allocate_picture(s, picture_ptr, 0, 1)<  0)
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> +
>>   int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
>>   {
>>       int len, nb_components, i, width, height, pix_fmt_id;
>> +    AVFrame *picture_ptr;
>> +    MXpegDecodeContext *mxctx = (s->avctx->codec_id == CODEC_ID_MXPEG) ? s->avctx->priv_data : 0;
>>      
> I'd prefer if you did this a bit differently.  More on that below.
>
>    
>> +    if (mxctx) {
>> +        mxctx->got_sof_data = 0;
>> +        if (s->lossless) {
>> +            av_log(s->avctx, AV_LOG_ERROR, "Lossless mode doesn't available in MxPEG\n");
>>      
> Bad grammar.
>    

Sorry.

>>           s->coefs_finished[c] |= 1;
>>           if(s->flipped) {
>> @@ -794,12 +862,15 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int nb_components, int Ah, i
>>
>>       for(mb_y = 0; mb_y<  s->mb_height; mb_y++) {
>>           for(mb_x = 0; mb_x<  s->mb_width; mb_x++) {
>> +            const int copy_mb = use_mxm_bitmask&&  !get_bits1(&mxm_bitmask_gb);
>>      
> Indexing the bit directly is probably faster than using get_bits.
> Something like bitmask[idx>>3]&  (128>>  (idx&  7)) should do it.
> Also, what happens if the bitmask is smaller than the full frame?
>    
> If this is allowed, the easiest way to handle it is probably to allocate
> a larger bitmask buffer and pad it with zeros.
>
>    

Ok. I will try.

>> +static int mxpeg_decode_mxm(AVCodecContext *avctx, MXpegDecodeContext *mxctx,
>> +                            char *buf, int len)
>> +{
>> +    int32_t  bitmask_size, i;
>> +    uint32_t mb_count;
>>      
> Why do you use sized types here?  For local variables it is almost
> always better to use plain int or unsigned int types.
>
>    

I'm not sure that sizeof(int) >= 4 in all architectures supported by 
FFmpeg. Correct me if I wrong.


>> +    av_freep(&mxctx->comment_buffer);
>> +    mxctx->comment_buffer = buf;
>> +    mxctx->mxm_bitmask = buf + 12;
>> +
>> +    mxctx->mb_width  = AV_RL16(&buf[4]);
>> +    mxctx->mb_height = AV_RL16(&buf[6]);
>> +    mb_count = (uint32_t)mxctx->mb_width * mxctx->mb_height;
>>      
> Why the cast?
>
>    

This is also about sizeof(int).


>> +    if (!mb_count || mb_count>  0x7FFFFFF8) {
>>      
> This looks strange.  What condition are you actually trying to check for?
>
>    

This is a check for possible overflow of (mb_count + 7) value.


>> diff --git a/libavcodec/mjpegdec.h b/libavcodec/mjpegdec.h
>> index 7baa5dc..24597d5 100644
>> --- a/libavcodec/mjpegdec.h
>> +++ b/libavcodec/mjpegdec.h
>> @@ -107,6 +107,19 @@ typedef struct MJpegDecodeContext {
>>       unsigned int ljpeg_buffer_size;
>>   } MJpegDecodeContext;
>>
>> +typedef struct MXpegDecodeContext {
>> +    MJpegDecodeContext jpgctx;
>> +    int got_sof_data; /* true if SOF data successfully parsed */
>> +    uint8_t *comment_buffer; /* temporary comment buffer */
>> +    uint8_t *mxm_bitmask; /* bitmask buffer */
>> +    int32_t bitmask_size; /* size of bitmask */
>> +    uint8_t has_complete_frame; /* true if has complete frame */
>> +    uint8_t *completion_bitmask; /* completion bitmask of macroblocks */
>> +    int mb_width, mb_height; /* size of picture in MB's from MXM header */
>>      
> Are these different from the fields with the same names in MJpegDecodeContext?
>
>    
>> +    AVFrame pictures[2]; /* pictures array */
>> +    int picture_index; /* index of current picture, other picture is reference */
>> +} MXpegDecodeContext;
>>      
> I would add these, apart from pictures[], to the end of
> MJpegDecodeContext and make the existing picture there an array of two
> instead.
>
>    

Ok. I will add these fields in MJpegDecodeContext.