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

[Michael Niedermayer]

On Thu, Nov 04, 2010 at 05:39:40PM +0300, Anatoly Nenashev wrote:
> On 04.11.2010 03:16, Michael Niedermayer wrote:
>> On Tue, Nov 02, 2010 at 01:58:21PM +0300, Anatoly Nenashev wrote:
>>    
>>>   int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
>>>   {
>>>       int len, nb_components, i, width, height, pix_fmt_id;
>>> @@ -342,14 +376,12 @@ int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
>>>               s->avctx->pix_fmt = PIX_FMT_GRAY16;
>>>       }
>>>
>>> -    if(s->picture.data[0])
>>> -        s->avctx->release_buffer(s->avctx,&s->picture);
>>> -
>>> -    s->picture.reference= 0;
>>> -    if(s->avctx->get_buffer(s->avctx,&s->picture)<  0){
>>> -        av_log(s->avctx, AV_LOG_ERROR, "get_buffer() failed\n");
>>> -        return -1;
>>> +    if (s->avctx->codec_id == CODEC_ID_MXPEG&&  !s->got_picture) {
>>> +        if (mxpeg_allocate_picture(s)<  0) return -1;
>>> +    } else {
>>> +        if (mjpeg_allocate_picture(s)<  0) return -1;
>>>       }
>>>      
>> looks like s->picture.reference will b wrong for the first pic
>>
>>
>>    
>
> No, s->picture.reference will be always 1 for MxPEG frames. But I've  
> reimplemented this code to be more clean. See attachment.
>> [...]
>>    
>>> +static int mxpeg_decode_mxm(MJpegDecodeContext *s, char *buf, int len)
>>> +{
>>> +    int32_t  mb_width, mb_height, bitmask_size, i;
>>> +    uint32_t mb_count;
>>> +
>>> +    mb_width  = AV_RL16(&buf[4]);
>>> +    mb_height = AV_RL16(&buf[6]);
>>> +    mb_count = (uint32_t)mb_width*mb_height;
>>> +
>>> +    if (!mb_count || mb_count>  0x7FFFFFF8) {
>>> +        av_log(s->avctx, AV_LOG_ERROR, "MXM wrong macroblocks count");
>>> +        return AVERROR(EINVAL);
>>> +    }
>>> +
>>> +    bitmask_size = (int32_t)(mb_count + 7)>>  3;
>>> +
>>> +    if (bitmask_size>  (len - 12)) {
>>> +        av_log(s->avctx, AV_LOG_ERROR, "MXM bitmask is not complete");
>>> +        return AVERROR(EINVAL);
>>> +    }
>>> +
>>> +    av_freep(&s->mxctx.comment_buffer);
>>> +    s->mxctx.comment_buffer = buf;
>>> +    s->mxctx.mxm_bitmask = buf + 12;
>>> +
>>> +    if (!s->got_picture&&  s->picture.data[0]) {
>>> +        if (mxpeg_allocate_picture(s)<  0) return -1;
>>> +        s->got_picture = 1;
>>> +    }
>>>      
>> allocating pictures in the comment parsing code is not a good idea IMHO
>>    
> I don't agree.
> There are 3 types of frames in MxPEG stream:
> 1) clear JPEG which contains SOF, DHT, DQT, SOS  - may be the first  
> frame in stream or (in new cameras firmware) may be periodically 
> available.
> 2) Pseudo-I frame which contains SOF, DHT, DQT, MXM bitmask (in COM),  
> SOS - must be periodically available but doesn't need to contain full  
> parts of image.
> 3) P-frame which contains MXM bitmask, SOS and optional DHT, DQT - just  
> usual frame.
>
> So where I must allocate picture for the last frame type? I've decided  
> to do it in MXM parser.
>
>
>> also maybe it would be easier to use reget_buffer() ?
>>
>>
>>    
>
> No, I think reget_buffer is a bad idea. We don't need to copy all  
> reference frame but only non-changed part of it.
> And we had talking about it in August. It was your idea to remove  
> reget_buffer from this code to reject full image copying.

both methods have their advantages and disadvantages.
i dont care which is implemented but it should be done cleanly
allocating the frame in the comment parsing code is not ok.
Allocating a picture currently implicates that a SOF has been successfully
parsed as the later code checks for this.
Simply allocating a picture without successfully running the SOF parsing but
maybe having it run just to the middle where it failed could lead to
exploitable security issues



[...]
> @@ -112,6 +112,9 @@ av_cold int ff_mjpeg_decode_init(AVCodecContext *avctx)
>      if (avctx->codec->id == CODEC_ID_AMV)
>          s->flipped = 1;
>  
> +    if (avctx->codec_id == CODEC_ID_MXPEG)
> +        memset(&s->mxctx, 0, sizeof(s->mxctx));

why is this needed?


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

Democracy is the form of government in which you can choose your dictator
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101106/008ce857/attachment.pgp>