Bug 186547 – [WPE] Build getUserMedia support

• Component: New Bugs
• Status: RESOLVED FIXED

[Thibault Saunier]

This is currently built only for GTK, this add supports to WPE and mediastream
layout tests are also run in WPE now.

[Thibault Saunier]

Created attachment 342474 [details]
[WPE] Build getUserMedia support

[Alejandro G. Castro]

Comment on attachment 342474 [details]
[WPE] Build getUserMedia support

View in context: https://bugs.webkit.org/attachment.cgi?id=342474&action=review

LGTM, some small comments inlined.

> Source/WebCore/ChangeLog:19
> +        no implemented in the Cairo backend itself.

s/no/now/

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:217
> +    copy = cairo_image_surface_create(CAIRO_FORMAT_ARGB32,

You can use RefPtr here. In WebKit we usually define the variales when used.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:221
> +    cr = cairo_create(copy);

Ditto here to avoid the explicit cairo_destroy later.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:245
> +    return imageData;

Is there a leak of the surface? If you return the RefPtr probably it would be fixed and you can avoid the destroy in the early return.

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:334
> +    GRefPtr<WebKitMediaStreamSrc> self = adoptGRef(WEBKIT_MEDIA_STREAM_SRC(gst_object_get_parent(parent)));

Does this mean we get the parent of the parent?

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:354
> +    RELEASE_ASSERT(gst_element_add_pad(GST_ELEMENT(self), GST_PAD(ghostpad)));

Do we really want to crash on release? Or we could control the situation log a gstreamer error and leave?

[Thibault Saunier]

Created attachment 342539 [details]
patch.

(In reply to Alejandro G. Castro from comment #2)
> Comment on attachment 342474 [details]
> [WPE] Build getUserMedia support
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342474&action=review
> 
> LGTM, some small comments inlined.
> 
> > Source/WebCore/ChangeLog:19
> > +        no implemented in the Cairo backend itself.
> 
> s/no/now/

Fixed.

> > Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:217
> > +    copy = cairo_image_surface_create(CAIRO_FORMAT_ARGB32,
> 
> You can use RefPtr here. In WebKit we usually define the variales when used.

Fixed.

> > Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:221
> > +    cr = cairo_create(copy);
> 
> Ditto here to avoid the explicit cairo_destroy later.

Fixed.

> > Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:245
> > +    return imageData;
> 
> Is there a leak of the surface? If you return the RefPtr probably it would
> be fixed and you can avoid the destroy in the early return.

Done.

> > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:334
> > +    GRefPtr<WebKitMediaStreamSrc> self = adoptGRef(WEBKIT_MEDIA_STREAM_SRC(gst_object_get_parent(parent)));
> 
> Does this mean we get the parent of the parent?

Yes, that is correct, this is what we want.

> > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:354
> > +    RELEASE_ASSERT(gst_element_add_pad(GST_ELEMENT(self), GST_PAD(ghostpad)));
> 
> Do we really want to crash on release? Or we could control the situation log
> a gstreamer error and leave?

Done just that.

[EWS Watchlist]

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


ERROR: Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:238:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:355:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 13 files


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

[EWS Watchlist]

Comment on attachment 342539 [details]
patch.

Attachment 342539 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/8148434

New failing tests:
accessibility/smart-invert-reference.html

[EWS Watchlist]

Created attachment 342554 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Thibault Saunier]

Created attachment 342558 [details]
Fixed styling issues.

[Thibault Saunier]

Created attachment 342575 [details]
Added alsa devel as required dependency in the install-dependencies script

[EWS Watchlist]

Comment on attachment 342575 [details]
Added alsa devel as required dependency in the install-dependencies script

Attachment 342575 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/8152173

New failing tests:
js/dom/JSON-stringify.html

[EWS Watchlist]

Created attachment 342606 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Thibault Saunier]

(In reply to Build Bot from comment #9)
> Comment on attachment 342575 [details]
> Added alsa devel as required dependency in the install-dependencies script
> 
> Attachment 342575 [details] did not pass mac-debug-ews (mac):
> Output: http://webkit-queues.webkit.org/results/8152173
> 
> New failing tests:
> js/dom/JSON-stringify.html

Unrelated.

[Alejandro G. Castro]

Comment on attachment 342575 [details]
Added alsa devel as required dependency in the install-dependencies script

View in context: https://bugs.webkit.org/attachment.cgi?id=342575&action=review

LGTM, let's try it, I added a couple of minor modifications.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:217
> +    copy = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32,

Can we do auto copy = ... here?

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:221
> +    cr = cairo_create(copy.get());

I think we can do: auto cr = adoptRef(cairo_create(copy.get())); And remove the destroy of the cr later.

[Thibault Saunier]

Created attachment 342652 [details]
Patch

[Thibault Saunier]

Created attachment 342653 [details]
[WPE] Build getUserMedia support

[WebKit Commit Bot]

Comment on attachment 342653 [details]
[WPE] Build getUserMedia support

Rejecting attachment 342653 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 342653, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=342653&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=186547&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 342653 from bug 186547.
Fetching: https://bugs.webkit.org/attachment.cgi?id=342653
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 17 diffs from patch file(s).
patching file ChangeLog
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/gtk/TestExpectations
patching file LayoutTests/platform/wpe/TestExpectations
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/PlatformWPE.cmake
patching file Source/WebCore/SourcesGTK.txt
patching file Source/WebCore/SourcesWPE.txt
patching file Source/WebCore/platform/GStreamer.cmake
patching file Source/WebCore/platform/graphics/ImageBuffer.cpp
patching file Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp
patching file Source/WebCore/platform/graphics/gtk/ImageBufferGtk.cpp
patching file Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp
patching file Source/WebKit/ChangeLog
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/ChangeLog.rej
patching file Source/WebKit/SourcesWPE.txt
patching file Source/cmake/OptionsWPE.cmake
patching file Tools/wpe/install-dependencies
patch unexpectedly ends in middle of line

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/8164931

[Thibault Saunier]

Created attachment 342673 [details]
Patch

Fix a ChangeLog weirdness.

[WebKit Commit Bot]

Comment on attachment 342673 [details]
Patch

Clearing flags on attachment: 342673

Committed r232796: <https://trac.webkit.org/changeset/232796>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41097214>