[FFmpeg-devel] [PATCH 3/3] cllc: Implement ARGB support

[Reimar Döffinger]

On Sun, Aug 12, 2012 at 02:43:34PM -0400, Derek Buitenhuis wrote:
> On 11/08/2012 8:41 AM, Reimar Döffinger wrote:
> > On Fri, Aug 10, 2012 at 01:07:11PM -0400, Derek Buitenhuis wrote:
> >> +        pred[0] += code;
> >> +        dst[0]   = pred[0];
> >> +
> >> +        /* Skip the components if they are  entirely transparent */
> >> +        if (dst[0]) {
> >> +            /* Red */
> >> +            UPDATE_CACHE(bits, gb);
> >> +            GET_VLC(code, bits, gb, vlc[1].table, 7, 2);
> >> +
> >> +            pred[1] += code;
> >> +            dst[1]   = pred[1];
> >> +
> >> +            /* Green */
> >> +            UPDATE_CACHE(bits, gb);
> >> +            GET_VLC(code, bits, gb, vlc[2].table, 7, 2);
> >> +
> >> +            pred[2] += code;
> >> +            dst[2]   = pred[2];
> >> +
> >> +            /* Blue */
> >> +            UPDATE_CACHE(bits, gb);
> >> +            GET_VLC(code, bits, gb, vlc[3].table, 7, 2);
> >> +
> >> +            pred[3] += code;
> >> +            dst[3]   = pred[3];
> >> +        } else {
> >> +            dst[1] = 0;
> >> +            dst[2] = 0;
> >> +            dst[3] = 0;
> > 
> > Those writes are not optimal.
> > You could use
> > AV_COPY32(dst, pred);
> > and
> > AV_ZERO(dst);
> > respectively.
> > However I admit the AV_COPY might needlessly make gcc write the data
> > out to stack...
> 
> This seems to have made it no faster, but require more changes. I elect to
> keep it as-is.
> 
> > 
> >> +    top_left[0]  = dst[0];
> >> +    
> >> +    /* Only stash components if they are not transparent */
> >> +    if (top_left[0]) {
> >> +        top_left[1] = dst[1];
> >> +        top_left[2] = dst[2];
> >> +        top_left[3] = dst[3];
> >> +    }
> > 
> > AV_COPY might make sense here as well:
> > if (dst[0]) AV_COPY32(top_left, dst);
> > else top_left[0] = 0;
> 
> Same.

Out of curiosity what do you mean by "require more changes"?
Did I miss something? I admit I also proposed it because it seems to me
to be less code, and simpler to read and understand what goes on on top.
Not that it really matters much, it's not an attempt to push my
"solution", I just like to understand why people disagree.