Bug 35783 – [GStreamer] Use ImageBuffer API to do painting

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

The current ::paint method of the player contains calls to Cairo. It would be better, I think, to add support for GStreamer buffers in cairo/ImageBuffer and use it in the painting code. That way other ports could reuse the player backend more easily. They'd need to add GStreamer buffer->Image support in their painting abstraction though.

[Philippe Normand]

Created attachment 50098 [details]
proposed patch

[WebKit Review Bot]

Attachment 50098 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/gstreamer/ImageGStreamer.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/gstreamer/ImageGStreamer.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/graphics/gstreamer/ImageGStreamer.cpp:27:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 50099 [details]
proposed patch

[Eric Seidel (no email)]

Comment on attachment 50099 [details]
proposed patch

is + * Copyright (C) 2010 Igalia S.L a company name?

[Philippe Normand]

(In reply to comment #4)
> (From update of attachment 50099 [details])
> is + * Copyright (C) 2010 Igalia S.L a company name?

Yes Igalia S.L is the company I work for.

[Philippe Normand]

Created attachment 50222 [details]
proposed patch

Previous patch with cairo.h include removed from the header of the player

[Gustavo Noronha (kov)]

Comment on attachment 50222 [details]
proposed patch

 49     private:
 50         cairo_surface_t* m_surface;
 51         RefPtr<BitmapImage> m_image;
 52 
 53     };

Drop the needless line.

 1172     bool success;
 1173     ImageGStreamer gstImage(m_buffer, &success);

I don't like this. It looks very non-idiomatic. I would prefer to have the createClass pattern used in, say, ScrollBar. That means create a static function that instantiates the object, and does whatever checking/initialization is needed, then return the object or NULL. You can decide on success based on the return value being NULL or not. Also, you seem to be leaking the image, aren't you? Should it be OwnPtr?

 1180     bool useLowQualityScale = false;
 1181     context->drawImage(reinterpret_cast<Image*>(gstImage.image().get()), styleColorSpace,
 1182                        rect, op, useLowQualityScale);

This is not very readable. You're saying 'Do not use low quality scale' by passing a 'useLowQualityScale' variable =D. I understand it's a boolean, of course, but I got confused reading this. I'd drop the variables, give values directly.

[Philippe Normand]

Created attachment 50282 [details]
proposed patch

[WebKit Review Bot]

Attachment 50282 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/gstreamer/ImageGStreamer.cpp:43:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Eric, could you share some thoughts on this? =)

[Philippe Normand]

Eric, we'd like your opinion on this patch, please :)

[Eric Carlson]

Comment on attachment 50282 [details]
proposed patch

> + *
> + * 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.

[Philippe Normand]

Thanks Eric for your careful review ;) Due to the amount of changes asked I preferred to rework the patch so it can get a last pass from you and/or Gustavo.

I adressed all your comments excepted:

> 
> > +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.
> 

It seems I can't make the destructor private because in the case where the public createImage() returns NULL, ~PassRefPtr() is called which somehow triggers the ~ImageGStreamer(). So in this context the destructor can't be private.

But agreed, I made the constructor 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? 
> 

Those are done in ImageCairo.cpp in the draw method.

[Philippe Normand]

Comment on attachment 50282 [details]
proposed patch

Attaching a reworked patch

[Philippe Normand]

Created attachment 51276 [details]
updated patch

I also explicitely named ImageGStreamer.cpp to ImageGStreamerCairo.cpp
and added the checks for CAIRO platform.

[WebKit Review Bot]

Attachment 51276 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/gstreamer/ImageGStreamer.h:43:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Comment on attachment 51276 [details]
updated patch

> + *
> + * 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

[Philippe Normand]

(In reply to comment #17)

> > +
> > +    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?
> 

Yes ;) It can be sparse RGB too, like GST_VIDEO_FORMAT_BGRx or GST_VIDEO_CAPS_xRGB depending on endian-ness. CAIRO_FORMAT_RGB24 is little/big endian dependant as mentionned in the VideoSinkGStreamer.cpp file, where the sinktemplate of the sink is defined.

Should I really assert anyway?

[Eric Carlson]

(In reply to comment #18)
> (In reply to comment #17)
> > > +
> > > +    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?
> > 
> 
> Yes ;) It can be sparse RGB too, like GST_VIDEO_FORMAT_BGRx or
> GST_VIDEO_CAPS_xRGB depending on endian-ness. CAIRO_FORMAT_RGB24 is little/big
> endian dependant as mentionned in the VideoSinkGStreamer.cpp file, where the
> sinktemplate of the sink is defined.
> 
> Should I really assert anyway?

Completely up to you, as I noted in my previous review I know essentially *nothing* about Cairo or GStreamer ;-)

[Philippe Normand]

Landed as r56354. Thanks for the review! I didn't add the ASSERT() for GST_VIDEO_FORMAT_RGB finally ;)