[FFmpeg-devel] [PATCH] Fix non-mod16 libtheora encoding

[David Conrad]

On May 22, 2009, at 3:54 PM, Michael Niedermayer wrote:

> On Fri, May 22, 2009 at 05:52:53AM -0400, David Conrad wrote:
>> On May 21, 2009, at 10:15 PM, Michael Niedermayer wrote:
>>
>>> On Thu, May 21, 2009 at 05:04:00PM -0700, Baptiste Coudurier wrote:
>>>> David Conrad wrote:
>>>>> On May 21, 2009, at 6:14 PM, Baptiste Coudurier wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> David Conrad wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Currently using non-mod16 sizes produces garbage.
>>>>>>>
>>>>>>>
>>>>>>> ------------------------------------------------------------------------
>>>>>>>
>>>>>>> commit 7af4d351600878e1c37467837d8d10c226e2b805
>>>>>>> Author: David Conrad <lessen42 at gmail.com>
>>>>>>> Date:   Thu May 21 17:38:44 2009 -0400
>>>>>>>
>>>>>>>  Fix libtheora encoding for non-mod16 sizes
>>>>>>>
>>>>>>> diff --git a/libavcodec/libtheoraenc.c b/libavcodec/ 
>>>>>>> libtheoraenc.c
>>>>>>> index dbc98e3..ef8b6aa 100644
>>>>>>> --- a/libavcodec/libtheoraenc.c
>>>>>>> +++ b/libavcodec/libtheoraenc.c
>>>>>>> @@ -87,12 +87,12 @@ static av_cold int  
>>>>>>> encode_init(AVCodecContext*
>>>>>>> avc_context)
>>>>>>>
>>>>>>>   /* Set up the theora_info struct */
>>>>>>>   theora_info_init( &t_info );
>>>>>>> -    t_info.width = avc_context->width;
>>>>>>> -    t_info.height = avc_context->height;
>>>>>>> +    t_info.width = (avc_context->width + 15) & 0xFFFFFFF0;
>>>>>>> +    t_info.height = (avc_context->height + 15) & 0xFFFFFFF0;
>>>>>>>   t_info.frame_width = avc_context->width;
>>>>>>>   t_info.frame_height = avc_context->height;
>>>>>>>   t_info.offset_x = 0;
>>>>>>> -    t_info.offset_y = 0;
>>>>>>> +    t_info.offset_y = avc_context->height & 0xf;
>>>>>>>   /* Swap numerator and denominator as time_base in  
>>>>>>> AVCodecContext
>>>>>>> gives the
>>>>>>>    * time period between frames, but theora_info needs the
>>>>>>> framerate.  */
>>>>>>>   t_info.fps_numerator = avc_context->time_base.den;
>>>>>>> @@ -186,8 +186,8 @@ static int encode_frame(
>>>>>>>       return -1;
>>>>>>>   }
>>>>>>>
>>>>>>> -    t_yuv_buffer.y_width = avc_context->width;
>>>>>>> -    t_yuv_buffer.y_height = avc_context->height;
>>>>>>> +    t_yuv_buffer.y_width = (avc_context->width + 15) &  
>>>>>>> 0xFFFFFFF0;
>>>>>>> +    t_yuv_buffer.y_height = (avc_context->height + 15) &  
>>>>>>> 0xFFFFFFF0;
>>>>>>>   t_yuv_buffer.y_stride = frame->linesize[0];
>>>>>>>   t_yuv_buffer.uv_width = t_yuv_buffer.y_width / 2;
>>>>>>>   t_yuv_buffer.uv_height = t_yuv_buffer.y_height / 2;
>>>>>>
>>>>>> I believe a failure is more appropriate and a message saying that
>>>>>> libtheora does not support non mod 16 width/height.
>>>>>
>>>>> Why not simply do the little bit of work libtheora won't do?
>>>>
>>>> Because acting on behalf of the user is bad.
>>>>
>>>> Inform the user of the problem, and let him handle it by either  
>>>> pad or
>>>> crop
>>>> as he wishes, is far better IMHO.
>>>
>>> if libtheora doesnt support non mod 16 then yes i fully agree that  
>>> we
>>> should
>>> fail rather than clip something at random behind the users back
>>
>> As discussed on IRC, the resulting behaviour is no different than  
>> libx264
>> or any other macroblock codec I'm aware of - the coded size is  
>> padded to
>> mod16 and the extra up to 15 pixels simply aren't displayed. The only
>> difference is that libtheora requires the caller to do this manually.
>
> then the patch is ok, though theora is silly

No disagreement, applied.