[webkit-reviews] review granted: [Bug 35783] [GStreamer] Use ImageBuffer API to do painting : [Attachment 51276] updated patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 35783: [GStreamer] Use ImageBuffer API to do painting
https://bugs.webkit.org/show_bug.cgi?id=35783

Attachment 51276: updated patch
https://bugs.webkit.org/attachment.cgi?id=51276&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
> + *
> + * You should have received a copy of the GNU Library General Public License

> + * aint with this library; see the file COPYING.LIB.  If not, write to
> 
Typo here, "aint" should be "along" (see
https://bugs.webkit.org/show_bug.cgi?id=36442).


> +	   PassRefPtr<BitmapImage> image() {
> +	       ASSERT(m_image);
> +	       return m_image.get();
> +	   }
>
The brace beginning a function definition should be on its own line.


> + *
> + * You should have received a copy of the GNU Library General Public License

> + * aint with this library; see the file COPYING.LIB.  If not, write to
>
Same typo here too.


> +#include "ImageGStreamer.h"
> +
> +#include "GOwnPtr.h"
> +
Don't need a blank line between these.


> +
> +    cairo_format_t cairoFormat;
> +    if (format == GST_VIDEO_FORMAT_ARGB || format == GST_VIDEO_FORMAT_BGRA)
> +	   cairoFormat = CAIRO_FORMAT_ARGB32;
> +    else
> +	   cairoFormat = CAIRO_FORMAT_RGB24;
> +
In the previous review I recommended asserting the format is
GST_VIDEO_FORMAT_RGB before assigning CAIRO_FORMAT_RGB24. I suspect Cairo will
support other pixel formats in the future, if so someone will want to optimize
and not always convert to RGB. Do you disagree?


r=me with these minor changes