[Pixman] [RFC PATCH 2/3] utils.[ch]: add fence_image_create_bits ()

[Pekka Paalanen]

On Thu, 14 May 2015 00:25:45 +0100
"Ben Avison" <bavison at riscosopen.org> wrote:

> On Fri, 08 May 2015 13:45:36 +0100, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > +pixman_image_t *
> > +fence_image_create_bits (pixman_format_code_t format,
> > +                         int min_width, int height,
> > +                         pixman_bool_t stride_fence)
> 
> Since you picked me up on subtleties of coding style, it's only fair that
> I point out that in function definitions, the arguments should either all
> be on one line, or one per line (with identifier names justified).

Yes, thanks.

> > +    pixels = fence_malloc (stride * (unsigned)height);
> 
> I wonder if it wouldn't be better to reduce the size here by one page if
> stride_fence==false - otherwise you've got a gap of one page between the
> end of the last row of pixel data and the end fence.

Yeah, a good idea.

I'm not completely sure we need stride_fence boolean at all, but I
suppose it's good to have in case we find that it breaks things it
shouldn't, so we can add quirks if we can't fix the underlying problem.
But, all that is arguable, because if we ever need such a quirk, it
means Pixman is broken on that arch. It becomes a question if it was
already broken, or did we break it.

> > +pixman_image_t *
> > +fence_image_create_bits (pixman_format_code_t format,
> > +                         int min_width, int height,
> > +                         pixman_bool_t stride_fence)
> > +{
> > +    return pixman_image_create_bits (format, width, height, NULL, 0);
> > +}
> 
> Perhaps a comment here to say that the auto-malloced pixel array is also
> auto-freed when the image reference count drops to zero, so there's no
> need for a destroy function in this case?

I can add that, sure.


Thanks,
pq