Bug 35783 - [GStreamer] Use ImageBuffer API to do painting
Summary: [GStreamer] Use ImageBuffer API to do painting
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-05 00:20 PST by Philippe Normand
Modified: 2010-03-22 12:39 PDT (History)
4 users (show)

See Also:


Attachments
proposed patch (10.08 KB, patch)
2010-03-05 08:50 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (10.05 KB, patch)
2010-03-05 08:54 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (10.62 KB, patch)
2010-03-08 08:16 PST, Philippe Normand
gns: review-
gns: commit-queue-
Details | Formatted Diff | Diff
proposed patch (10.76 KB, patch)
2010-03-09 02:08 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
updated patch (9.90 KB, patch)
2010-03-22 04:57 PDT, Philippe Normand
eric.carlson: review+
eric.carlson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2010-03-05 00:20:56 PST
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.
Comment 1 Philippe Normand 2010-03-05 08:50:32 PST
Created attachment 50098 [details]
proposed patch
Comment 2 WebKit Review Bot 2010-03-05 08:51:29 PST
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.
Comment 3 Philippe Normand 2010-03-05 08:54:23 PST
Created attachment 50099 [details]
proposed patch
Comment 4 Eric Seidel (no email) 2010-03-05 12:31:10 PST
Comment on attachment 50099 [details]
proposed patch

is + * Copyright (C) 2010 Igalia S.L a company name?
Comment 5 Philippe Normand 2010-03-08 00:05:19 PST
(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.
Comment 6 Philippe Normand 2010-03-08 08:16:27 PST
Created attachment 50222 [details]
proposed patch

Previous patch with cairo.h include removed from the header of the player
Comment 7 Gustavo Noronha (kov) 2010-03-08 13:44:50 PST
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.
Comment 8 Philippe Normand 2010-03-09 02:08:44 PST
Created attachment 50282 [details]
proposed patch
Comment 9 WebKit Review Bot 2010-03-09 02:11:43 PST
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.
Comment 10 Gustavo Noronha (kov) 2010-03-10 07:51:35 PST
Eric, could you share some thoughts on this? =)
Comment 11 Philippe Normand 2010-03-19 04:04:21 PDT
Eric, we'd like your opinion on this patch, please :)
Comment 12 Eric Carlson 2010-03-19 07:46:35 PDT
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.
Comment 13 Philippe Normand 2010-03-22 04:53:06 PDT
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.
Comment 14 Philippe Normand 2010-03-22 04:56:30 PDT
Comment on attachment 50282 [details]
proposed patch

Attaching a reworked patch
Comment 15 Philippe Normand 2010-03-22 04:57:51 PDT
Created attachment 51276 [details]
updated patch

I also explicitely named ImageGStreamer.cpp to ImageGStreamerCairo.cpp
and added the checks for CAIRO platform.
Comment 16 WebKit Review Bot 2010-03-22 05:02:02 PDT
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.
Comment 17 Eric Carlson 2010-03-22 08:40:07 PDT
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
Comment 18 Philippe Normand 2010-03-22 09:01:39 PDT
(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?
Comment 19 Eric Carlson 2010-03-22 09:10:30 PDT
(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 ;-)
Comment 20 Philippe Normand 2010-03-22 12:39:23 PDT
Landed as r56354. Thanks for the review! I didn't add the ASSERT() for GST_VIDEO_FORMAT_RGB finally ;)