Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comcast modifications #15

Merged
merged 7 commits into from
Dec 8, 2015
Merged

Conversation

emutavchi
Copy link

I haven't tried this with WPE, so it may be specific to qtwebkit we use. Please review.

@@ -1725,12 +1740,12 @@ void AppendPipeline::connectToAppSinkFromAnyThread(GstPad* demuxerSrcPad)
connectToAppSink(demuxerSrcPad);
} else {
// Call connectToAppSink() in the main thread and wait.
PadInfo* info = new PadInfo(demuxerSrcPad, this);
info->lock();
GMutexLocker lock(&m_padAddRemoveMutex);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't compile, needs to be WTF::GMutexLocker lock(....)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include WTF's GMutexLocker.h header, then WTF::GMutexLocker<GMutex> lock(m_....)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@philn
Copy link

philn commented Dec 4, 2015

I tested this and I see a bunch of

ERROR webkitmediaplayer MediaPlayerPrivateGStreamerMSE.cpp:1416:setAppendStage: Invalid append stage transition Ongoing --> Ongoing

now. The state machine needs fixing.

@philn
Copy link

philn commented Dec 4, 2015

Is the setAppendStage error fixed in this new version?

@emutavchi
Copy link
Author

Yes, the 'Ongoing --> Ongoing' is fixed. Previous pull request missed some changes, this one should be ok.

@@ -213,6 +213,9 @@ MediaPlayerPrivateGStreamer::~MediaPlayerPrivateGStreamer()
ASSERT(bus);
gst_bus_set_sync_handler(bus.get(), nullptr, nullptr, nullptr);
g_signal_handlers_disconnect_matched(m_pipeline.get(), G_SIGNAL_MATCH_DATA, 0, 0, nullptr, nullptr, this);
// BRCM specific, flush to unlock aud/vid filters
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put this in a ifdef

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please suggest the define to hide this under.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#if WPE_BACKEND(BCM_NEXUS)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WPE_BACKEND is not available in WebCore and WPE_BACKEND_BCM_NEXUS is defined only for WPE module, changing that seems to be out of current task scope.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove this change from pull request so we can proceed with other changes. This patch is needed when brcm decoders limit_buffering is enabled, so you should be good without it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok let's remove that patch for now then.
The fix I'm suggesting is to define WTF_PLATFORM_BRCM to 1 during the build, so that you can use #if PLATFORM(BRCM) in the code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

philn added a commit that referenced this pull request Dec 8, 2015
[MSE] leak fixes and improvements
@philn philn merged commit cc52b3b into WebPlatformForEmbedded:master Dec 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants