Bug 186547 - [WPE] Build getUserMedia support
Summary: [WPE] Build getUserMedia support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Thibault Saunier
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-11 15:51 PDT by Thibault Saunier
Modified: 2018-06-13 12:29 PDT (History)
8 users (show)

See Also:


Attachments
[WPE] Build getUserMedia support (22.13 KB, patch)
2018-06-11 15:56 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
patch. (22.27 KB, patch)
2018-06-12 08:29 PDT, Thibault Saunier
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.17 MB, application/zip)
2018-06-12 10:00 PDT, EWS Watchlist
no flags Details
Fixed styling issues. (22.26 KB, patch)
2018-06-12 10:33 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Added alsa devel as required dependency in the install-dependencies script (22.93 KB, patch)
2018-06-12 12:09 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-sierra (3.07 MB, application/zip)
2018-06-12 15:51 PDT, EWS Watchlist
no flags Details
Patch (81.73 KB, patch)
2018-06-13 06:56 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
[WPE] Build getUserMedia support (23.26 KB, patch)
2018-06-13 07:45 PDT, Thibault Saunier
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (22.90 KB, patch)
2018-06-13 10:43 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thibault Saunier 2018-06-11 15:51:57 PDT
This is currently built only for GTK, this add supports to WPE and mediastream
layout tests are also run in WPE now.
Comment 1 Thibault Saunier 2018-06-11 15:56:55 PDT
Created attachment 342474 [details]
[WPE] Build getUserMedia support
Comment 2 Alejandro G. Castro 2018-06-12 02:01:38 PDT
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?
Comment 3 Thibault Saunier 2018-06-12 08:29:02 PDT
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.
Comment 4 EWS Watchlist 2018-06-12 08:32:04 PDT
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.
Comment 5 EWS Watchlist 2018-06-12 10:00:27 PDT
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
Comment 6 EWS Watchlist 2018-06-12 10:00:29 PDT
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
Comment 7 Thibault Saunier 2018-06-12 10:33:34 PDT
Created attachment 342558 [details]
Fixed styling issues.
Comment 8 Thibault Saunier 2018-06-12 12:09:40 PDT
Created attachment 342575 [details]
Added alsa devel as required dependency in the install-dependencies script
Comment 9 EWS Watchlist 2018-06-12 15:51:14 PDT
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
Comment 10 EWS Watchlist 2018-06-12 15:51:15 PDT
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
Comment 11 Thibault Saunier 2018-06-12 16:18:20 PDT
(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.
Comment 12 Alejandro G. Castro 2018-06-13 00:26:41 PDT
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.
Comment 13 Thibault Saunier 2018-06-13 06:56:51 PDT
Created attachment 342652 [details]
Patch
Comment 14 Thibault Saunier 2018-06-13 07:45:27 PDT
Created attachment 342653 [details]
[WPE] Build getUserMedia support
Comment 15 WebKit Commit Bot 2018-06-13 10:38:52 PDT
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 commit-queue@webkit.org...
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
Comment 16 Thibault Saunier 2018-06-13 10:43:42 PDT
Created attachment 342673 [details]
Patch

Fix a ChangeLog weirdness.
Comment 17 WebKit Commit Bot 2018-06-13 11:22:47 PDT
Comment on attachment 342673 [details]
Patch

Clearing flags on attachment: 342673

Committed r232796: <https://trac.webkit.org/changeset/232796>
Comment 18 WebKit Commit Bot 2018-06-13 11:22:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Radar WebKit Bug Importer 2018-06-13 12:29:44 PDT
<rdar://problem/41097214>