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
_______________________________________________
Gnash-dev mailing list
Gnash-dev@gnu.org
http://lists.gnu.org/mailman/listinfo/gnash-dev
