Bug 115355

Summary: [gstreamer] Prevent race when pad caps is set on gstreamer player
Product: WebKit Reporter: Andre Moreira Magalhaes <andrunko>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, commit-queue, darin, eric.carlson, glenn, gns, jer.noble, kadam, kbalazs, menard, mrobinson, ossy, pnormand, thiago.sousa.santos, zarvai
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
pnormand: review+, pnormand: commit-queue-
Patch
pnormand: review+, commit-queue: commit-queue-
Patch none

Description Andre Moreira Magalhaes 2013-04-29 08:00:48 PDT
Prevent race when pad caps is set on gstreamer player.

Patch to follow.
Comment 1 Andre Moreira Magalhaes 2013-04-29 08:05:06 PDT
Created attachment 200012 [details]
Patch
Comment 2 Philippe Normand 2013-05-13 03:17:18 PDT
Comment on attachment 200012 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        No new tests (OOPS!).

Commit queue will complain here. Please remove this line. Also the entry format is wrong. Should be something like

bug title
bug link

Reviewed by...

Change description
Comment 3 Andre Moreira Magalhaes 2013-05-20 09:39:20 PDT
Created attachment 202281 [details]
Patch

> > Source/WebCore/ChangeLog:12
> > +        No new tests (OOPS!).
> 
> Commit queue will complain here. Please remove this line. Also the entry format is wrong. Should be something like
> 
> bug title
> bug link
> 
> Reviewed by...
> 
> Change description
Done
Comment 4 Andre Moreira Magalhaes 2013-05-20 10:30:49 PDT
Thanks all for the review, all patches are updated now with comments applied.
Comment 5 Philippe Normand 2013-05-27 03:45:02 PDT
Comment on attachment 202281 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:30
> +#include <glib.h>

Too bad enums can't be forward-declared, it would have been good to avoid this include.
Comment 6 WebKit Commit Bot 2013-05-31 00:59:29 PDT
Comment on attachment 202281 [details]
Patch

Rejecting attachment 202281 [details] from commit-queue.

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

Last 500 characters of output:
383 with fuzz 1 (offset 51 lines).
1 out of 4 hunks FAILED -- saving rejects to file Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp.rej
patching file Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h
Hunk #1 succeeded at 27 with fuzz 1.
Hunk #2 succeeded at 123 (offset 14 lines).

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

Full output: http://webkit-queues.appspot.com/results/664222
Comment 7 Philippe Normand 2013-05-31 01:03:56 PDT
Can you please rebase the patch with the "Reviewed by" field updated and mark it cq? ? Thanks :)
Comment 8 Andre Moreira Magalhaes 2013-06-04 10:16:16 PDT
Created attachment 203709 [details]
Patch

(In reply to comment #7)
> Can you please rebase the patch with the "Reviewed by" field updated and mark it cq? ? Thanks :)

Done, rebased patch attached.
Comment 9 WebKit Commit Bot 2013-06-04 11:07:07 PDT
Comment on attachment 203709 [details]
Patch

Clearing flags on attachment: 203709

Committed r151175: <http://trac.webkit.org/changeset/151175>
Comment 10 WebKit Commit Bot 2013-06-04 11:07:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Csaba Osztrogon√°c 2013-06-05 00:23:45 PDT
It broke the build with gstreamer 0.1 (Qt WK2 perf bot and Qt MIPS bot):

/data/buildbot/mips-1/qt-linux-mipsel-mips32r2-release/build/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp: In member function 'virtual WebCore::IntSize WebCore::MediaPlayerPrivateGStreamerBase::naturalSize() const':
/data/buildbot/mips-1/qt-linux-mipsel-mips32r2-release/build/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:171:32: error: invalid conversion from 'const GMutex* {aka const _GMutex*}' to 'GMutex* {aka _GMutex*}' [-fpermissive]


/usr/mipsel-linux-gnu/include/glib-2.0/glib/gthread.h:157:17: error:   initializing argument 1 of 'void g_mutex_lock(GMutex*)' [-fpermissive]
/data/buildbot/mips-1/qt-linux-mipsel-mips32r2-release/build/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:173:34: error: invalid conversion from 'const GMutex* {aka const _GMutex*}' to 'GMutex* {aka _GMutex*}' [-fpermissive]


Isn't gstreamer 1.0 mandatory long time ago? If no, it needs buildfix.
If yes, Qt build system should be fixed to require gstreamer 1.0 installed.
( I mean removing the 0.1 fallback introduced in https://trac.webkit.org/changeset/144208 )
Comment 12 Philippe Normand 2013-06-05 00:34:18 PDT
Qt should switch to gst 1.x, there's a bug about that, but this is not my call :)

Someone should indeed fix this build error, we need gst 0.10 support building. Why did that patch pass on the qt EWS?
Comment 13 Csaba Osztrogon√°c 2013-06-05 00:47:37 PDT
(In reply to comment #12)
> Qt should switch to gst 1.x, there's a bug about that, but this is not my call :)
> 
> Someone should indeed fix this build error, we need gst 0.10 support building. Why did that patch pass on the qt EWS?

All Qt bots (EWS bots too) already use gst 1.0 (from the related 
thread on webkit-dev mailing list), but the MIPS and WK2 perf bot.
Comment 14 Philippe Normand 2013-06-05 02:17:52 PDT
https://trac.webkit.org/r151203
Comment 15 Balazs Kelemen 2013-06-09 16:01:20 PDT
Could you provide some information about why did you only fixed it for the old version? Thanks.