Summary: | [GStreamer] Use ImageBuffer API to do painting | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | eric.carlson, gustavo, webkit.review.bot, xan.lopez | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Attachments: |
|
Description
Philippe Normand
2010-03-05 00:20:56 PST
Created attachment 50098 [details]
proposed patch
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.
Created attachment 50099 [details]
proposed patch
Comment on attachment 50099 [details]
proposed patch
is + * Copyright (C) 2010 Igalia S.L a company name?
(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. Created attachment 50222 [details]
proposed patch
Previous patch with cairo.h include removed from the header of the player
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.
Created attachment 50282 [details]
proposed patch
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.
Eric, could you share some thoughts on this? =) Eric, we'd like your opinion on this patch, please :) 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. 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 on attachment 50282 [details]
proposed patch
Attaching a reworked patch
Created attachment 51276 [details]
updated patch
I also explicitely named ImageGStreamer.cpp to ImageGStreamerCairo.cpp
and added the checks for CAIRO platform.
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 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 (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? (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 ;-) Landed as r56354. Thanks for the review! I didn't add the ASSERT() for GST_VIDEO_FORMAT_RGB finally ;) |