Bug 30925 - [GTK] Black artifacts in youtube.com/html5
Summary: [GTK] Black artifacts in youtube.com/html5
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL: http://www.youtube.com/html5
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-29 13:47 PDT by Xan Lopez
Modified: 2009-11-11 03:25 PST (History)
4 users (show)

See Also:


Attachments
screeshot (399.28 KB, image/png)
2009-10-29 13:49 PDT, Xan Lopez
no flags Details
patch, step 1 (2.66 KB, patch)
2009-10-29 15:59 PDT, Benjamin Otte
jmalonzo: review-
Details | Formatted Diff | Diff
fix (3.69 KB, patch)
2009-10-29 16:04 PDT, Benjamin Otte
no flags Details | Formatted Diff | Diff
fix (4.94 KB, patch)
2009-11-02 11:44 PST, Benjamin Otte
no flags Details | Formatted Diff | Diff
fix (4.59 KB, patch)
2009-11-02 11:51 PST, Benjamin Otte
jmalonzo: review+
jmalonzo: commit-queue-
Details | Formatted Diff | Diff
updated ChangeLog entry (4.78 KB, patch)
2009-11-10 04:28 PST, Benjamin Otte
eric: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
rebased against trunk (4.82 KB, patch)
2009-11-10 11:57 PST, Benjamin Otte
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xan Lopez 2009-10-29 13:47:38 PDT
Using r50298, cairo 1.8.0, X server 1.5.3 with intel drivers 2.5.0, I get black artifacts playing the youtube demo version using HTML5 (see URL in the bug). Screenshot coming.
Comment 1 Xan Lopez 2009-10-29 13:49:26 PDT
Created attachment 42136 [details]
screeshot
Comment 2 Benjamin Otte 2009-10-29 15:59:54 PDT
Created attachment 42157 [details]
patch, step 1
Comment 3 Benjamin Otte 2009-10-29 16:04:28 PDT
Created attachment 42158 [details]
fix

These two patches get rid of the improper alignment as exhibited in the bug.

According to Simon Fraser the right thing to do in MediaPlayerPrivate::paint() is to make the video fill the given rectangle completely and not try to keep any aspect ratio.

FWIW, at least the Qt and the win port have the same issue. I'm not sure if we should file new bugs for that or use this one?
Comment 4 Jan Alonzo 2009-10-30 16:01:32 PDT
Comment on attachment 42157 [details]
patch, step 1

> +        Using cairo_paint() is faster, does not produce artefacts and is less
> +        code.

Typo: artefacts -> artifacts.

Patch looks fine. But it doesn't apply cleanly anymore. Can you please update so the commit bot can commit it?

r- because patch needs rebasing.
Comment 5 Jan Alonzo 2009-10-30 16:02:31 PDT
Comment on attachment 42158 [details]
fix

LGTM. Can you please update this one as well for cq+? Thanks.
Comment 6 Benjamin Otte 2009-11-02 11:44:48 PST
Created attachment 42328 [details]
fix

Turns out using cairo_paint() is a bad idea for the gradients you get in the corner with bilinear filtering, which are bad when upscaling the image. So I removed that part and used the old cairo_fill() logic and set CAIRO_EXTEND_PAD on the source.
Other than that, the patch is unchanged.
Comment 7 Benjamin Otte 2009-11-02 11:51:38 PST
Created attachment 42329 [details]
fix

This time with only one ChangeLog entry...
Comment 8 Jan Alonzo 2009-11-10 01:36:23 PST
Comment on attachment 42329 [details]
fix

> +2009-11-02  Benjamin Otte  <otte@gnome.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Paint the video to the given size.
> +
> +        It's the job of the callers to keep track of aspect ratio.
> +        RenderVideo.cpp does it for the <video> element.

r=me. ChangeLog is missing bug link and title though, so someone would need to fix it before landing.
Comment 9 Benjamin Otte 2009-11-10 04:28:16 PST
Created attachment 42858 [details]
updated ChangeLog entry
Comment 10 Eric Seidel (no email) 2009-11-10 11:03:42 PST
Comment on attachment 42858 [details]
updated ChangeLog entry

LGTM too.  You should be aware of:
prepare-ChangeLog --bug 12345
which will automatically add the title and url for the bug to the ChangeLog.
Comment 11 WebKit Commit Bot 2009-11-10 11:12:58 PST
Comment on attachment 42858 [details]
updated ChangeLog entry

Rejecting patch 42858 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Eric Seidel', '--force']" exit_code: 1
Last 500 characters of output:
aphics/gtk/MediaPlayerPrivateGStreamer.cpp   |   51 ++++----------------
 2 files changed, 23 insertions(+), 42 deletions(-)


-------------------------------------------------------------------
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp
Hunk #1 FAILED at 634.
Hunk #2 FAILED at 655.
2 out of 2 hunks FAILED -- saving rejects to file WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp.rej
Comment 12 Benjamin Otte 2009-11-10 11:57:44 PST
Created attachment 42883 [details]
rebased against trunk
Comment 13 Jan Alonzo 2009-11-11 03:08:17 PST
Comment on attachment 42883 [details]
rebased against trunk

> +2009-11-02  Benjamin Otte  <otte@gnome.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [GTK] Black artifacts in youtube.com/html5
> +
> +        Paint the video to the given size. It's the job of the callers to keep
> +        track of aspect ratio. RenderVideo.cpp does it for the <video>
> +        element.
> +        https://bugs.webkit.org/show_bug.cgi?id=30925
> +

Try to put the bug link after the title next time.
Comment 14 WebKit Commit Bot 2009-11-11 03:25:04 PST
Comment on attachment 42883 [details]
rebased against trunk

Clearing flags on attachment: 42883

Committed r50798: <http://trac.webkit.org/changeset/50798>
Comment 15 WebKit Commit Bot 2009-11-11 03:25:08 PST
All reviewed patches have been landed.  Closing bug.