Re: [Gnash-dev] Flash HD (H.264) video decoding acceleration

[Benjamin Wolsey]

Partial review of the VAAPI patch. I'm most concerned with what the
patch does for maintainability. There are ever fewer developers with
less and less time, and it's already difficult to maintain all the GUIs,
media handlers, and renderers. So it's pretty crucial that new code
doesn't add more complexity to the design. That's why I'm particularly
in favour of the GnashImage change if it means we can replace BitmapInfo
and use ImageLocation for all renderers.

>  
> +  boost::shared_ptr<GnashTexture> getCachedTexture(GnashImage *frame)
> 

There are coding style guidelines at
http://wiki.gnashdev.org/CodingStyle that address things like formatting
of function signatures. 

> +      // Look for a texture with the same dimensions and type
> +      std::list< boost::shared_ptr<GnashTexture> >::iterator it;
> +      for (it = _cached_textures.begin(); it !=
> _cached_textures.end(); it++) {
> +         if ((*it)->width() == frame->width() &&
> +             (*it)->height() == frame->height() &&
> +             (*it)->internal_format() ==
> frameFormat.internal_format() &&
> +             (*it)->format() == frameFormat.format() &&
> +             (*it)->flags() == frameFlags)
> +             break;
> +      }
> 

std::list::find with a function object predicate would be tidier.


> +    for (int i = 0; i < _render_textures.size(); i++)

size_t for container sizes, or there is a (legitimate) compiler
warrning.

> 
> @@ -99,13 +108,19 @@ public:
>      /// @param height   The height of the image in pixels.
>      /// @param pitch    The pitch (rowstride) of the image in bytes.
>      /// @param type     The ImageType of the image.
> -    GnashImage(int width, int height, int pitch, ImageType type);
> +    GnashImage(int width, int height, int pitch, ImageType type,
> +               ImageLocation location = GNASH_IMAGE_CPU);

There's a class called BitmapInfo that's used currently to store a
reference to a bitmap held in the Renderer class. It's used for gradient
fills and images. It was supposed to allow storing the bitmap in a form
native to the renderer, which in OGL is presumably a texture. I've never
liked the design because it forces the coder to decide which of two
incompatible bitmap classes to use.

Would it be feasible to use the GnashImage with a GPU 'location' to
replace the BitmapInfo class? The location of the image would be much
more transparent then. The AGG renderer merely stores the bitmap in the
same format so it could stay as a CPU-located GnashImage; no idea what
cairo does; OGL can use GPU storage possibly for all GnashImages, as
long as there is access to the bitmap data when required.

> +    int i;
> +    for (i = 0; gl_errors[i].str; i++) {
> 
> +    const char *end;
> +    int name_len, n;
> +
> +    if (name == NULL || ext == NULL)
> +        return false;
> +
> +    end = ext + strlen(ext);
> +    name_len = strlen(name);

Unless deprecated C headers like string.h are included, it should be
std::strlen, or some compilers will reject it. I guess this code has
been copied from some C files.

The following class (and some others) have funny indentation and order
of variables (public - protected - private) is inconsistent with the
rest of Gnash.

> +/// OpenGL texture abstraction
> +class DSOEXPORT GnashTexture {
> +    unsigned int       _width;
> +    unsigned int       _height;
> +    unsigned int       _texture;
> +    GnashTextureFormat _format;
> +
> +    /// OpenGL texture state
> +    struct TextureState {
> +       unsigned int    old_texture;
> +       unsigned int    was_enabled : 1;
> +       unsigned int    was_bound   : 1;
> +    }                  _texture_state;
> +
> +protected:
> +    unsigned int       _flags;
> +
> +private:
> +    bool init();
> +
> +public:
> +    GnashTexture(unsigned int width, unsigned int height, ImageType
> type);
> +    virtual ~GnashTexture();
> +
> +    /// Return texture flags
> +    unsigned int flags() const
> +       { return _flags; }
> +
> +    /// Return texture width
> +    unsigned int width() const
> +       { return _width; }

> 
> +#include "GnashVaapiImage.h"
> +#include "vaapi.h"

#include <vaapi.h> ?

> 
> +#include <time.h>

#include <ctime>, or in fact:

> +/// Get current value of microsecond timer
> +static boost::uint64_t get_ticks_usec(void)
> +{

clocktime::getTicks in libbase/ClockTime.h does this.

> +/* SIMD versions of DSPContext.float_to_int16_interleave() needs
> input
> +   and output buffers aligned to 16-byte boundaries */
> +#define NEEDS_ALIGNED_MEMORY 1
> +

Is there a problem always using av_malloc always for this?

> +
> +               if (output)
> +                   av_free(output);
> +               else
> +                   delete [] output;
>                 return NULL;

Typo, but if we always use av_malloc there's no need for the
conditionals anyway.

> +       else if (NEEDS_ALIGNED_MEMORY)
> +       {
> +           boost::uint8_t* newOutput = new boost::uint8_t[outSize];
> +           memcpy(newOutput, output, outSize);
> +           outPtr = reinterpret_cast<boost::int16_t*>(newOutput);
> +       }

I suppose the extra (std::!)memcpy is the problem. We need a closer look
at the design there.

--
Use Gnash, the GNU Flash Player!
http://www.gnu.org/software/gnash/

Benjamin Wolsey, Software Developer - http://benjaminwolsey.de

signature.asc
Description: Dies ist ein digital signierter Nachrichtenteil