[webkit-reviews] review granted: [Bug 35783] [GStreamer] Use ImageBuffer API to do painting : [Attachment 50282] proposed 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 50282: proposed patch
https://bugs.webkit.org/attachment.cgi?id=50282&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> + * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR
> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
> + * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */

I think this is the wrong version, don't you want the one with "THIS SOFTWARE
IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS"?


> +
> +    IntSize size(width, height);
> +
This local is only used in the call to the ImageGStreamer constructor, you
could pass "size(width, height)" instead.


> +    if (format == GST_VIDEO_FORMAT_ARGB || format == GST_VIDEO_FORMAT_BGRA)
> +	   cairoFormat = CAIRO_FORMAT_ARGB32;
> +    else
> +	   cairoFormat = CAIRO_FORMAT_RGB24;
> +
Now that the GstBuffer is being created in a completely different class it
seems unsafe to assume you have RGB if you don't have ARGB or BGRA. Worth an
assert()?


> +ImageGStreamer::ImageGStreamer(GstBuffer*& buffer, IntSize& size,
cairo_format_t& cairoFormat)
> +    : m_surface(0)
> +    , m_image(0)
> +{
> +    m_surface = cairo_image_surface_create_for_data(GST_BUFFER_DATA(buffer),
cairoFormat,
> +						       size.width(),
size.height(),
> +						       4 * size.width());
> +
Won't "4 * size.width()" only work for some widths and/or pixel formats? I
think it would be more future proof to calculate the stride with
cairo_format_stride_for_width instead.


> +    if (m_surface)
> +	   m_image = BitmapImage::create(m_surface);
> +}
The Cairo documentation says cairo_image_surface_create_for_data() always
returns a valid pointer, even if the allocation fails or the stride value is
invalid. Seems like cairo_surface_status() is the correct way to check for
failure. Might be worth adding an assert to catch failures here since
ImageGStreamer::image() will assert later when someone tries to get the image.


> +    if (cairo_surface_get_reference_count(m_surface))
> +	   cairo_surface_destroy(m_surface);
> +
Is it possible for m_surface to be non-nil and have a reference count? If not,
I think it would be better to test the pointer directly assert the reference
count when it is non-nil.


> +class ImageGStreamer : public RefCounted<ImageGStreamer> {
> +    public:
> +	   static PassRefPtr<ImageGStreamer> createImage(GstBuffer*);
> +
> +	   ImageGStreamer(GstBuffer*&, IntSize&, cairo_format_t&);
> +	   ~ImageGStreamer();
> +
> +	   PassRefPtr<BitmapImage> image();
> +
> +    private:
> +
The constructor and destructor should be private.


> -
> -    cairo_save(cr);
> -
> -    // translate and scale the context to correct size
> -    cairo_translate(cr, rect.x(), rect.y());
> -    cairo_scale(cr, static_cast<double>(rect.width()) / width,
static_cast<double>(rect.height()) / height);
> -
> -    // And paint it.
> -    cairo_set_source_surface(cr, src, 0, 0);
> -    cairo_pattern_set_extend(cairo_get_source(cr), CAIRO_EXTEND_PAD);
> -    cairo_rectangle(cr, 0, 0, width, height);
> -    cairo_fill(cr);
> -    cairo_restore(cr);
>
Why are you able to remove the positioning and sizing code, was it unnecessary
before? 

r=me with these changes.