Hi,
On Sat, Jan 28, 2012 at 12:46 AM, Diego Biurrun <[email protected]> wrote:
> On Fri, Jan 27, 2012 at 07:54:36AM +0800, Ronald S. Bultje wrote:
>> Also convert to yasm, add a loop to add_png_paeth_pred() to handle
>> the case where bpp>4 (e.g. RGB48), and add a SSE2 version for
>> add_bytes_l2().
>
> I'm with Mans on this one, the patch would benefit from splitting.
As Mans said: done.
>> +void ff_pngdsp_init(PNGDSPContext *dsp)
>> +{
>> + dsp->add_bytes_l2 = add_bytes_l2_c;
>> + dsp->add_png_paeth_prediction = ff_add_png_paeth_prediction;
>
> nit: align
Was already done.
>> --- /dev/null
>> +++ b/libavcodec/pngdsp.h
>> @@ -0,0 +1,40 @@
>> +/*
>> + * PNG image format
>> + * Copyright (c) 2003 Fabrice Bellard
>
> This does not look like Fabrice's code.
Fixed.
>> +typedef struct {
>> + /* this might write to dst[w] */
>> + void (*add_png_paeth_prediction)(uint8_t *dst, uint8_t *src,
>> + uint8_t *top, int w, int bpp);
>> + void (*add_bytes_l2)(uint8_t *dst/*align 16*/, uint8_t *src1/*align
>> 16*/,
>> + uint8_t *src2/*align 16*/, int w);
>> +
>> +} PNGDSPContext;
>
> I know we disagree about typedeffing structs, but please at least give
> it a name so that it is not typedeffed anonymously.
Fixed.
>> --- /dev/null
>> +++ b/libavcodec/x86/pngdsp-init.c
>> @@ -0,0 +1,58 @@
>> +/*
>> + * PNG image format
>> + * Copyright (c) 2003 Fabrice Bellard
>
> ditto
Fixed.
>> +void ff_pngdsp_x86_init(PNGDSPContext *dsp)
>> +{
>> +#if HAVE_YASM
>> + int flags = av_get_cpu_flags();
>> +
>> +#if ARCH_X86_32
>> + if (flags & AV_CPU_FLAG_MMX) {
>> + dsp->add_bytes_l2 = ff_add_bytes_l2_mmx;
>> + }
>> +#endif
>> +
>> + if (flags & AV_CPU_FLAG_MMX2) {
>> + dsp->add_png_paeth_prediction = ff_add_png_paeth_prediction_mmx2;
>> + }
>> +
>> + if (flags & AV_CPU_FLAG_SSE2) {
>> + dsp->add_bytes_l2 = ff_add_bytes_l2_sse2;
>> + }
>> +
>> + if (flags & AV_CPU_FLAG_SSSE3) {
>> + dsp->add_png_paeth_prediction = ff_add_png_paeth_prediction_ssse3;
>> + }
>
> Doesn't ARCH_X86_64 imply SSE2? What's the point of the ARCH_X86_32
> #ifdef?
>
> Also, I'd drop the pointless {} and make this more concise.
I forgot doing this, it causes a lot of conflicts when applying each
successive patch.
Ronald
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
