Re: [Gnash-commit] gnash gui/gtk.cpp gui/gtksup.h gui/Makefile.am ...

[Bastiaan Jacques]

On Friday 13 October 2006 18:38, Hannes Mayr wrote:
> CVSROOT:      /sources/gnash
> Module name:  gnash
> Changes by:   Hannes Mayr <bik>       06/10/13 16:38:55
>
> Modified files:
>       gui            : gtk.cpp gtksup.h Makefile.am gtk_glue.h
>       .              : ChangeLog
> Added files:
>       gui            : gtk_glue_agg.cpp gtk_glue_agg.h
>
> Log message:
>       Added support for GTK with AGG renderer

It looks very nice; well done. :) Some minor comments are below:

> Index: gui/gtk_glue_agg.cpp
> ===================================================================
> RCS file: gui/gtk_glue_agg.cpp
> diff -N gui/gtk_glue_agg.cpp
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ gui/gtk_glue_agg.cpp      13 Oct 2006 16:38:55 -0000      1.1

> +GtkAggGlue::GtkAggGlue()
> +{
> +}
> +
> +GtkAggGlue::~GtkAggGlue()
> +{
> +
> +}
> +
> +bool
> +GtkAggGlue::init(int argc, char **argv[])
> +{
> +    gdk_rgb_init();
> +
> +             _offscreenbuf_size      = 0;
> +             _offscreenbuf           = NULL;
> +             _agg_renderer       = NULL;
> +             _width                  = 0;
> +             _height                 = 0;
> +             _bpp                    = 0;

Considering initialising these members in the constructor:

GtkAggGlue::GtkAggGlue()
  : _offscreenbuf_size(0),
    _offscreenbuf(NULL) 
    // and so on
{
}


> +
> +    return true;
> +}
> +
> +void
> +GtkAggGlue::prepDrawingArea(GtkWidget *drawing_area)
> +{
> +    _drawing_area = drawing_area;
> +
> +    gtk_widget_get_size_request(_drawing_area, &_width, &_height);
> +
> +    _width   = (_width == -1) ? 0 : _width;
> +    _height  = (_height == -1) ? 0 : _height;

How can _width and _height ever be -1 if you set them to zero in init() 
and you check that they are greater than zero in 
setRenderHandlerSize()?


> +    _bpp             = gdk_visual_get_best_depth();
> +
> +}

> +void
> +GtkAggGlue::setRenderHandlerSize(int width, int height)
> +{
> +     assert(width>0);
> +     assert(height>0);
> +     assert(_agg_renderer!=NULL);
> +
> +     #define CHUNK_SIZE (100*100*(_bpp/8))
> +
> +     if (width == _width && height == _height)
> +        return;
> +
> +     int new_bufsize = width*height*(_bpp/8);
> +
> +     // TODO: At the moment we only increase the buffer and never
> decrease it. Should be +      // changed sometime.
> +     if (new_bufsize > _offscreenbuf_size) {
> +             new_bufsize = (int)(new_bufsize / CHUNK_SIZE + 1) * CHUNK_SIZE;
> +             _offscreenbuf   = (unsigned char *)realloc(_offscreenbuf,
> new_bufsize); +
> +             if (!_offscreenbuf) {
> +               log_msg("Could not allocate %i bytes for offscreen buffer:
> %s\n", +                              new_bufsize, strerror(errno)
> +                     );
> +                     return;
> +             }

In general, we prefer to use operator new instead of malloc() and 
friends. (You could write a wrapper that has the same functionality of 
realloc().)

> +             log_msg("GTK-AGG: %i bytes offscreen buffer allocated\n",
> new_bufsize); +               _offscreenbuf_size = new_bufsize;
> +     }
> +
> +  _width = width;
> +     _height = height;
> +
> +     ((render_handler_agg_base *)_agg_renderer)->init_buffer(
> +       _offscreenbuf,
> +             _offscreenbuf_size,
> +             _width,
> +             _height
> +     );

Please consider using C++-style casts instead. Also, it's not 
immediately obvious to me why the AGG render handler base class is 
used. Could you add a comment to clarify?

Bastiaan