[FFmpeg-devel] [PATCH] Fix MPEG video lowres crash

[Michael Niedermayer]

On Sat, Dec 18, 2010 at 02:56:31PM +0300, Anatoly Nenashev wrote:
> On 18.12.2010 05:08, Michael Niedermayer wrote:
>> [...]
>> There is avcodec_set_dimensions() which sets width/height correctly, the codec
>> should call that when being opened. The problem is av_find_stream_info() not
>> knowing the user specific lowres and the user is not able to set it as
>> av_find_stream_info() can add more streams
>> either way your code rounds wrong and might be exploitable
>>
>>
>>    
>
> Ok, I don't argue because I'm not a developer of this code  but I only  
> try to specify the problem.
>
>>>> MV=0 does not need the emu code but your change looks
>>>> like it would call it. I guess theres rather a oversight related to the length
>>>> of the MC filter
>>>>
>>>>
>>>>        
>>> This fix may by ugly but it was caused by SSSE3/MMX implementation of
>>> h264_chroma_mc4. The closest look at the code shows that if mc4 applyed
>>> in bottom macroblock's line of picture then overrun from source buffer
>>> is available even if MV=0. That issue can be fixed by enlarging
>>> picture's buffer size but I've decided that this is not a good solution
>>> corresponded to flag CODEC_FLAG_EMU_EDGE.
>>>      
>> see avcodec_align_dimensions2()
>>
>>    
>
> I've found the following line in avcodec_align_dimensions2():
> utils.c:188
>     if(s->codec_id == CODEC_ID_H264)
>         *height+=2; // some of the optimized chroma MC reads one line  
> too much
>
> Does it mean that other decoders which uses h264_chroma_mc must be added  
> here?

i thought that would simpler, yes


> Corresponded patch in attachment. The list is too long therefore
> probably I've forgotten some decoders.
> I don't like this fix because if somebody will add new decoder which  
> uses MPV_decode_mb then it will be necessary not to forget to add  
> decoder in this condition. The other way is just remove the condition  
> and do "*height+=2" by default.
> May be somebody has a better  idea.
>
>
>

>  utils.c |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 5b40a64762fd7eab5954f5b4426fc79f9197bf50  mpegvideo.patch
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 69d333e..0d74136 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -185,7 +185,16 @@ void avcodec_align_dimensions2(AVCodecContext *s, int *width, int *height, int l
>  
>      *width = FFALIGN(*width , w_align);
>      *height= FFALIGN(*height, h_align);
> -    if(s->codec_id == CODEC_ID_H264)
> +    if(s->codec_id == CODEC_ID_FLV1 ||
> +       s->codec_id == CODEC_ID_H263 ||
> +       s->codec_id == CODEC_ID_H263I ||
> +       s->codec_id == CODEC_ID_H264 ||
> +       s->codec_id == CODEC_ID_MPEG1VIDEO ||
> +       s->codec_id == CODEC_ID_MPEG2VIDEO ||
> +       s->codec_id == CODEC_ID_MPEG4 ||
> +       s->codec_id == CODEC_ID_MSMPEG4V1 ||
> +       s->codec_id == CODEC_ID_MSMPEG4V2 ||
> +       s->codec_id == CODEC_ID_MSMPEG4V3 ||)
>          *height+=2; // some of the optimized chroma MC reads one line too much

a || lowres seems simpler (if it works)


-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- 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/20101218/beca12df/attachment.pgp>