[FFmpeg-cvslog] r21736 - trunk/libavcodec/x86/dsputil_mmx.c

[Måns Rullgård]

David Conrad <lessen42 at gmail.com> writes:

> On Feb 12, 2010, at 2:53 PM, Michael Niedermayer wrote:
>
>> On Fri, Feb 12, 2010 at 07:18:53PM +0000, M?ns Rullg?rd wrote:
>>> Michael Niedermayer <michaelni at gmx.at> writes:
>>> 
>>>> On Fri, Feb 12, 2010 at 05:19:03PM +0000, M?ns Rullg?rd wrote:
>>>>> Michael Niedermayer <michaelni at gmx.at> writes:
>>>>> 
>>>>>> On Thu, Feb 11, 2010 at 03:30:46PM +0000, M?ns Rullg?rd wrote:
>>>>>>> Michael Niedermayer <michaelni at gmx.at> writes:
>>>>>>> 
>>>>>>>> On Wed, Feb 10, 2010 at 05:08:07PM -0500, David Conrad wrote:
>>>>>>>>> On Feb 10, 2010, at 4:16 PM, Alex Converse wrote:
>>>>>>>>> 
>>>>>>>>>> On Tue, Feb 9, 2010 at 9:02 PM, conrad <subversion at mplayerhq.hu> wrote:
>>>>>>>>>>> Author: conrad
>>>>>>>>>>> Date: Wed Feb 10 03:02:06 2010
>>>>>>>>>>> New Revision: 21736
>>>>>>>>>>> 
>>>>>>>>>>> Log:
>>>>>>>>>>> Enable SSE2 (put|avg)_pixels_16_sse2
>>>>>>>>>>> 
>>>>>>>>>>> SVQ1 chroma has been special-cased aligned to 16-bytes
>>>>>>>>>>> since at least r15466 Other architectures also assume
>>>>>>>>>>> 16-byte alignment here too but set STRIDE_ALIGN to 16.
>>>>>>>>>>> 
>>>>>>>>>>> Modified:
>>>>>>>>>>>  trunk/libavcodec/x86/dsputil_mmx.c
>>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> This broke vp6f:
>>>>>>>>>> http://fate.multimedia.cx/index.php?build_record=178551
>>>>>>>>>> http://fate.multimedia.cx/index.php?build_record=178488
>>>>>>>>> 
>>>>>>>>> Looks like vp5 and vp6 use width 16 pixel functions on chroma
>>>>>>>>> as well. I'd really prefer if SSE used 16-byte aligment like
>>>>>>>>> other architectures (first patch). Otherwise VP5/6 need to be
>>>>>>>>> special-cased as well (second patch), and potentially other
>>>>>>>>> codecs that FATE doesn't test, which gets extremely ugly
>>>>>>>>> fast.
>>>>>>>> 
>>>>>>>> i prefer the second patch
>>>>>>>> otherwise extensive benchmaring is needed
>>>>>>> 
>>>>>>> Maybe we could make it a codec capability flag or something instead of
>>>>>>> an ever-growing list of special cases.
>>>>>> 
>>>>>> yes, but it should go into a non public header, the user app has
>>>>>> no need to mess with it nor should we have to bump the ABI if we
>>>>>> decide to increase STRIDE_ALIGN one day and remove this
>>>>> 
>>>>> An app providing its own get_buffer would need to know, methinks...
>>>> 
>>>> hmm
>>>> 
>>>> maybe we should add avcodec_align_dimensions2() that provides
>>>> width/height/lumastride/chromastide alignment values
>>> 
>>> Fine with me, but I'd really like to see this fixed sooner rather than
>>> later.  FATE is in a very sorry state right now.
>>> 
>>>> It is not what we do, but why we do it that matters.
>>> 
>>> And when we do it...
>> 
>> ill revert the offending commit later today unless someone else fixed it
>> properly (too many things on my todo list for a proper fix from me)
>
> It's not a proper fix, but I pushed the special casing of vp5/6.
>
> Does something like the attached work to get rid of the format list in get_buffer()?
>
>
> commit 8e4a5e9f985d61f027fab869264a0bc3ced05ca3
> Author: David Conrad <lessen42 at gmail.com>
> Date:   Fri Feb 12 17:25:28 2010 -0500
>
>     Introduce an internal CODEC_CAP_ALIGN16_CHROMA for codecs that need 16-byte
>     chroma stride alignment.
>
> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> index 7ce019c..ef59dd6 100644
> --- a/libavcodec/internal.h
> +++ b/libavcodec/internal.h
> @@ -28,6 +28,12 @@
>  #include "avcodec.h"
>
>  /**
> + * Codec assumes 16-byte stride alignment for chroma
> + * Practically, this means put_pixels[0] is called on chroma planes
> + */
> +#define CODEC_CAP_ALIGN16_CHROMA 0x80000000

Defining this separately from the other CODEC_CAP_* flags is bound to
result in a collision sooner or later.

-- 
M?ns Rullg?rd
mans at mansr.com