[FFmpeg-devel] Yellow fever

[Ronald S. Bultje]

Hi,

On Mon, Aug 23, 2010 at 1:46 PM, M?ns Rullg?rd <mans at mansr.com> wrote:
> Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
>> On Sat, Aug 21, 2010 at 10:12:35AM +0100, M?ns Rullg?rd wrote:
>>> As you all have no doubt completely ignored, FATE turned much more
>>> yellow yesterday. ?The reason for this is that I disabled unaligned
>>> memory access fixup on those machines which do not support it in
>>> hardware (Alpha, ARMv5, MIPS). ?Most of these are failing a single
>>> test: lavfi-pixfmts_scale_le.
>>>
>>> The problem here is btr32ToY() and related functions in libswscale:
>>>
>>> #define BGR2Y(type, name, shr, shg, shb, maskr, maskg, maskb, RY, GY, BY, S)\
>>> static inline void name(uint8_t *dst, const uint8_t *src, long width, uint32_t *unused)\
>>> {\
>>> ? ? int i;\
>>> ? ? for (i=0; i<width; i++) {\
>>> ? ? ? ? int b= (((const type*)src)[i]>>shb)&maskb;\
>>> ? ? ? ? int g= (((const type*)src)[i]>>shg)&maskg;\
>>> ? ? ? ? int r= (((const type*)src)[i]>>shr)&maskr;\
>>> \
>>> ? ? ? ? dst[i]= (((RY)*r + (GY)*g + (BY)*b + (33<<((S)-1)))>>(S));\
>>> ? ? }\
>>> }
>>>
>>> These functions are called from hyscale(), like this:
>>>
>>> static inline void RENAME(hyscale)(SwsContext *c, uint16_t *dst, long dstWidth, const uint8_t *src, int srcW, int xInc,
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?const int16_t *hLumFilter,
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?const int16_t *hLumFilterPos, int hLumFilterSize,
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?uint8_t *formatConvBuffer,
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?uint32_t *pal, int isAlpha)
>>> {
>>> ? ? void (*toYV12)(uint8_t *, const uint8_t *, long, uint32_t *) = isAlpha ? c->alpToYV12 : c->lumToYV12;
>>> ? ? void (*convertRange)(uint16_t *, int) = isAlpha ? NULL : c->lumConvertRange;
>>>
>>> ? ? src += isAlpha ? c->alpSrcOffset : c->lumSrcOffset;
>>>
>>> ? ? if (toYV12) {
>>> ? ? ? ? toYV12(formatConvBuffer, src, srcW, pal);
>>> ? ? ? ? src= formatConvBuffer;
>>> ? ? }
>>> [...]
>>> }
>>>
>>> c->lumSrcOffset is either 1 or -1, meaning src will _always_ be
>>> unaligned.
>>
>> Not that it makes much of a difference overall, but I think you
>> are mistaken there (or I misunderstood you).
>> c->lumSrcOffset is one for PIX_FMT_RGB48LE, and 1 or -1 for the
>> 32_1 formats, but 0 for the other cases.
>> So only these (rather uncommon formats) need a fix, and IMO
>> for those even a solution with bad performance would be ok.
>
> I didn't dig very deeply, but it is 1 or -1 in the test that's
> failing, and that's bad enough. ?How would you propose fixing it?

if src_pix_fmt == RGB48LE (or any that gives &1==1), use a slow
version that does unaligned access. Else, use the current version.

Ronald