[FFmpeg-devel] [PATCH 1/2] MxPEG decoder
Anatoly Nenashev
anatoly.nenashev
Sat Nov 6 05:37:40 CET 2010
On 06.11.2010 06:01, Michael Niedermayer wrote:
> 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
>
>
>
>
As I have specified before there are frames which don't contain SOF
marker. I've named them P-frames.
If we have got Pseudo-I frame before then new picture for P-frame will
be allocated according to previously parsed params.
Otherwise no picture will be allocated. Allocating picture for P-frame
in SOS parser is too late because we can't get here due to s->got_picture=0.
So if you see the right place for P-frames' picture allocation please
tell me.
More information about the ffmpeg-devel
mailing list