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

[Bastiaan Jacques]

Hi Gwenole,

First off, I have to express some concern that apparently only
proprietary GPU drivers are supported for use with VA API -- at least,
for the codecs that are commonly used in Flash videos. The last
thing we want to do is encourage people to use proprietary drivers for
use with Gnash. I would be interested in merging the VA API integration
if we have some indication that the free GPU driver community (i.e.,
Nouveau or AMD-free) will soon be supported by VA API.

Still, some of the changes in the patch seem useful, even without VA
API integration.

Here are some comments from a preliminary read of the OpenGL part of the patch.

+      std::list< boost::shared_ptr<GnashTexture> >::iterator it;
+      for (it = _cached_textures.begin(); it != _cached_textures.end();
it++) {

Use ++it to save some CPU cycles.



+      }
+
+      // Otherwise, create one and empty cache because they may no
+      // longer be referenced
+      else {

Please put the comment inside the else{} block.


+       dynamic_cast<GnashVaapiTexture
*>(texture.get())->update(dynamic_cast<GnashVaapiImage
*>(frame)->surface());

Can you split this across multiple lines and check that your casting
succeeds (or, if you are sure that it will, use static_cast instead)?

+    gnash::point l, u;
+    m->transform(&l, point(bounds->get_x_min(), bounds->get_y_min()));
+    m->transform(&u, point(bounds->get_x_max(), bounds->get_y_max()));
+    const unsigned int w = u.x - l.x;
+    const unsigned int h = u.y - l.y;


I think it would be clearer to write:

  rect trans_bounds = bounds;
  m->transform(trans_bounds);
  boost::int32 w = trans_bounds.width();
  boost::int32 h = trans_bounds.height();


+// Returns a string representation of an OpenGL error
+static const char *gl_get_error_string(GLenum error)

Can you use gluErrorString() instead?

+static inline bool gl_do_check_error(int report)

report is used as a boolean, so it should be a bool.


+    D(bug("GnashTexture::GnashTexture()\n"));

Please use GNASH_PRINT_FUNCTION.

+GnashTexture::GnashTexture(unsigned int width, unsigned int height,
ImageType type)
+    : _width(width), _height(height), _texture(0), _format(type),
_flags(0)
+{
+    D(bug("GnashTexture::GnashTexture()\n"));
+
+    init();
+}

And what of init()'s return value?

+    struct TextureState {
+       unsigned int    old_texture;
+       unsigned int    was_enabled : 1;
+       unsigned int    was_bound   : 1;
+    }                  _texture_state;

Do performance numbers indicate that bitfields should be preferred over
clarity (i.e., using bool) here?

+    /// Note that this buffer MUST have the same _pitch, or unexpected
things
+    /// will happen.

Can you assert() on this condition in update()?

For sending future patches, it would be nice to have the VA API patch seperate
from other bugfixes.

Bastiaan