Mse backport #1

Open
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
@eocanha
Owner

eocanha commented May 4, 2016

No description provided.

@@ -401,7 +402,11 @@ MediaTime::ComparisonFlags MediaTime::compare(const MediaTime& rhs) const
return LessThan;
if (hasDoubleValue() && rhs.hasDoubleValue()) {
+#if USE(GSTREAMER)
+ if (fabs(m_timeValueAsDouble - rhs.m_timeValueAsDouble) <= FuzzinessThreshold)
+#else
if (m_timeValueAsDouble == rhs.m_timeValueAsDouble)

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

I think this will be a problem upstream. I remember Eric Carlson talking about something related to intervals that they were not keen to accept. Let's see if it's about this.

@calvaris

calvaris May 5, 2016

Collaborator

I think this will be a problem upstream. I remember Eric Carlson talking about something related to intervals that they were not keen to accept. Let's see if it's about this.

This comment has been minimized.

@eocanha

eocanha May 27, 2016

Owner

I know this kind of comparisons are a bit problematic. If I go back to exact comparisons, at least a test related to duration will fail. I remember I investigated the problem back then and it boiled down to a float vs. double comparison not being exact.

I tried to refine the above condition by checking for pure equality, then casting to the less accurate type (float) and comparing again if the previous condition fails, then falling back to the fuzzy comparison as a last resort. Anyway, that doesn't really fix the problem.

Part of the actual problem is that our private player code managing GStreamer times converts guint64 GStreamer times to float, while MediaTime manages (and compares) doubles. JavaScript uses MediaTime internally, and that's when the comparisons fail.

@eocanha

eocanha May 27, 2016

Owner

I know this kind of comparisons are a bit problematic. If I go back to exact comparisons, at least a test related to duration will fail. I remember I investigated the problem back then and it boiled down to a float vs. double comparison not being exact.

I tried to refine the above condition by checking for pure equality, then casting to the less accurate type (float) and comparing again if the previous condition fails, then falling back to the fuzzy comparison as a last resort. Anyway, that doesn't really fix the problem.

Part of the actual problem is that our private player code managing GStreamer times converts guint64 GStreamer times to float, while MediaTime manages (and compares) doubles. JavaScript uses MediaTime internally, and that's when the comparisons fail.

This comment has been minimized.

@calvaris

calvaris May 30, 2016

Collaborator

Ok, I don't see any problem in this if we open a bug to later review all interactions between GStreamer times and floating times by removing any use of float and ensuring we are using double as MediaTime

@calvaris

calvaris May 30, 2016

Collaborator

Ok, I don't see any problem in this if we open a bug to later review all interactions between GStreamer times and floating times by removing any use of float and ensuring we are using double as MediaTime

@@ -59,6 +59,7 @@ static int32_t signum(int64_t val)
}
const int32_t MediaTime::MaximumTimeScale = 0x7fffffffL;
+const double MediaTime::FuzzinessThreshold = 0.00003;

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Protect this with USE(GSTREAMER) too if it is only used by GStreamer ports.

@calvaris

calvaris May 5, 2016

Collaborator

Protect this with USE(GSTREAMER) too if it is only used by GStreamer ports.

@@ -118,7 +118,7 @@ class WTF_EXPORT_PRIVATE MediaTime {
static const int32_t DefaultTimeScale = 10000000;
static const int32_t MaximumTimeScale;
-
+ static const double FuzzinessThreshold;

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Protect this with USE(GSTREAMER) too if it is only used by GStreamer ports. Maybe a comment describing it could be useful too.

And do not remove the empty line.

@calvaris

calvaris May 5, 2016

Collaborator

Protect this with USE(GSTREAMER) too if it is only used by GStreamer ports. Maybe a comment describing it could be useful too.

And do not remove the empty line.

if (std::all_of(begin, end, [](RefPtr<SourceBuffer>& sourceBuffer) {
+#endif
return !sourceBuffer->hasCurrentTime();

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Why this?

@calvaris

calvaris May 5, 2016

Collaborator

Why this?

This comment has been minimized.

@eocanha

eocanha May 27, 2016

Owner

The right question is "how can it work for without this?". It seems to me that the interpretation of the spec that the current code is doing is a bit strange.

The old code translates to text as: "if each and every SourceBuffer doesn't have the current time, set HaveMetadata as readyState". Consequence: "If most SourceBuffers don't have the current time but one of them has it, the readyState may be something greater than HaveMetadata".

I think this is wrong, because the spec says: "If 'buffered' doesn't have the current time, set HaveMetadata as readyState", where 'buffered' translates to 'the intersection of audio and video', 'the buffered time ranges available on audio and video at the same time'. So, "if not 'all the SourceBuffers' have the current time, declare HaveMetadata", "if any SourceBuffer doesn't have the current time, declare HaveMetadata". That's what the proposed code does.

I've just kept the old code "as is" on the other ifdef branch to avoid breaking anything on non-GStreamer ports.

@eocanha

eocanha May 27, 2016

Owner

The right question is "how can it work for without this?". It seems to me that the interpretation of the spec that the current code is doing is a bit strange.

The old code translates to text as: "if each and every SourceBuffer doesn't have the current time, set HaveMetadata as readyState". Consequence: "If most SourceBuffers don't have the current time but one of them has it, the readyState may be something greater than HaveMetadata".

I think this is wrong, because the spec says: "If 'buffered' doesn't have the current time, set HaveMetadata as readyState", where 'buffered' translates to 'the intersection of audio and video', 'the buffered time ranges available on audio and video at the same time'. So, "if not 'all the SourceBuffers' have the current time, declare HaveMetadata", "if any SourceBuffer doesn't have the current time, declare HaveMetadata". That's what the proposed code does.

I've just kept the old code "as is" on the other ifdef branch to avoid breaking anything on non-GStreamer ports.

+ if (sync)
+ removeTimerFired();
+ else
+ m_removeTimer.startOneShot(0);
}

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Why?

@calvaris

calvaris May 5, 2016

Collaborator

Why?

This comment has been minimized.

@eocanha

eocanha May 27, 2016

Owner

While trying to answer this question, I noticed that this "sync" variant isn't actually needed. Something should be different between the downstream implementation and upstream, because upstream works fine without the "sync" variant. Removing.

@eocanha

eocanha May 27, 2016

Owner

While trying to answer this question, I noticed that this "sync" variant isn't actually needed. Something should be different between the downstream implementation and upstream, because upstream works fine without the "sync" variant. Removing.

@@ -519,7 +522,7 @@ void SourceBuffer::appendBufferInternal(unsigned char* data, unsigned size, Exce
evictCodedFrames(size);
// FIXME: enable this code when MSE libraries have been updated to support it.
-#if 0
+#if USE(GSTREAMER)

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Why?

@calvaris

calvaris May 5, 2016

Collaborator

Why?

This comment has been minimized.

@eocanha

eocanha May 30, 2016

Owner

Because the spec mandates to throw a QUOTA_EXCEEDED_ERR in this case. I don't know why the code was disabled before, but at leas in our case (GStreamer implementation), it should be enabled. I don't want to break the behaviour for other implementations, though.

@eocanha

eocanha May 30, 2016

Owner

Because the spec mandates to throw a QUOTA_EXCEEDED_ERR in this case. I don't know why the code was disabled before, but at leas in our case (GStreamer implementation), it should be enabled. I don't want to break the behaviour for other implementations, though.

+ for (HashMap<AtomicString, TrackBuffer>::iterator
+ it = m_trackBufferMap.begin(),
+ itEnd = m_trackBufferMap.end();
+ it != itEnd; ++it) {

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Please use complete names instead of it and itEnd

@calvaris

calvaris May 5, 2016

Collaborator

Please use complete names instead of it and itEnd

+ for (int i = trackRanges.length() - 1; i > 0; --i) {
+ const MediaTime priorEnd = trackRanges.end(i - 1);
+ const MediaTime currStart = trackRanges.start(i);
+ const MediaTime delta = currStart - priorEnd;

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Complete variable names, please

@calvaris

calvaris May 5, 2016

Collaborator

Complete variable names, please

@@ -582,6 +585,33 @@ void SourceBuffer::sourceBufferPrivateAppendComplete(SourceBufferPrivate*, Appen
if (isRemoved())
return;
+#if USE(GSTREAMER)
+ // FIXME: Remove some small gaps (less than lastFrameDuration) from reported buffered region.

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

I think we could be a bit more verbose here.

@calvaris

calvaris May 5, 2016

Collaborator

I think we could be a bit more verbose here.

@@ -1356,6 +1389,17 @@ void SourceBuffer::sourceBufferPrivateDidReceiveSample(SourceBufferPrivate*, Pas
it = m_trackBufferMap.add(trackID, TrackBuffer()).iterator;
TrackBuffer& trackBuffer = it->value;
+#if USE(GSTREAMER)
+ // FIXME: Hack to add fake range to fill start hole

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

We could be more specific in this comment. Finish with a period

@calvaris

calvaris May 5, 2016

Collaborator

We could be more specific in this comment. Finish with a period

@@ -37,6 +37,8 @@ AudioTrackPrivateGStreamer::AudioTrackPrivateGStreamer(GRefPtr<GstElement> playb
: TrackPrivateBaseGStreamer(this, index, pad)
, m_playbin(playbin)
{
+ // FIXME: Get a real ID here

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

What's wrong with this id? If there's nothing wrong we could remove the comment. Besides, comments end with a period.

@calvaris

calvaris May 5, 2016

Collaborator

What's wrong with this id? If there's nothing wrong we could remove the comment. Besides, comments end with a period.

This comment has been minimized.

@eocanha

eocanha May 30, 2016

Owner

This id is autogenerated by WebKit on MediaPlayerPrivateGStreamer::notifyPlayerOfAudio(). Being picky, we should use the track id present in the "tkhd" atom, but that would require some GStreamer/qtdemux cooperation. Autogeneration works fine by now, but I'll add the missing period.

@eocanha

eocanha May 30, 2016

Owner

This id is autogenerated by WebKit on MediaPlayerPrivateGStreamer::notifyPlayerOfAudio(). Being picky, we should use the track id present in the "tkhd" atom, but that would require some GStreamer/qtdemux cooperation. Autogeneration works fine by now, but I'll add the missing period.

+ m_pendingAsyncOperationsLock.lock();
+ while (m_pendingAsyncOperations) {
+ g_source_remove(GPOINTER_TO_UINT(m_pendingAsyncOperations->data));
+ m_pendingAsyncOperations = g_list_remove(m_pendingAsyncOperations, m_pendingAsyncOperations->data);
}

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

I'd iterate here to remove the sources and then do a g_[s]list_free at the end. You avoid all relinking operations.

@calvaris

calvaris May 5, 2016

Collaborator

I'd iterate here to remove the sources and then do a g_[s]list_free at the end. You avoid all relinking operations.

+{
+ m_pendingAsyncOperationsLock.lock();
+ ASSERT(m_pendingAsyncOperations);
+ m_pendingAsyncOperations = g_list_remove(m_pendingAsyncOperations, m_pendingAsyncOperations->data);

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Since you are always removing the first element, there is no need to do a find for data, just use g_[s]list_delete_link.

@calvaris

calvaris May 5, 2016

Collaborator

Since you are always removing the first element, there is no need to do a find for data, just use g_[s]list_delete_link.

+ virtual bool isMediaSource() const { return false; }
+
+ Mutex m_pendingAsyncOperationsLock;
+ GList* m_pendingAsyncOperations;

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

GList is a structure that is thought to be used forward and backwards. If you are going to use it only forwards you should use GSList and you save a pointer to the previous.

Now, both GSList and GList are more efficient if you prepend because accessing the last element needs traversing the whole list. If you are going to do such kind of operations you might want to use a GQueue. In this case we'd bring back the previous element pointer and use a bit more memory but the accesses would be faster.

Now, said all this and considering that you are in WebKit, why don't use a WTF structure instead of a GNOME one? Considering the parallel structures I would use a Deque.

@calvaris

calvaris May 5, 2016

Collaborator

GList is a structure that is thought to be used forward and backwards. If you are going to use it only forwards you should use GSList and you save a pointer to the previous.

Now, both GSList and GList are more efficient if you prepend because accessing the last element needs traversing the whole list. If you are going to do such kind of operations you might want to use a GQueue. In this case we'd bring back the previous element pointer and use a bit more memory but the accesses would be faster.

Now, said all this and considering that you are in WebKit, why don't use a WTF structure instead of a GNOME one? Considering the parallel structures I would use a Deque.

+ player->notifyDurationChanged();
+ return G_SOURCE_REMOVE;
+}
+

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

This is probably not needed. I'll explain later.

@calvaris

calvaris May 5, 2016

Collaborator

This is probably not needed. I'll explain later.

+
+ durationChanged();
+}
+

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

This is probably not needed either. I'll explain later.

@calvaris

calvaris May 5, 2016

Collaborator

This is probably not needed either. I'll explain later.

+
+ return MediaPlayerPrivateGStreamerBase::handleSyncMessage(message);
+}
+

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

And now the expected explanation. Why do we capture this message here to defer it to the main thread instead of just letting it go through to the main thread through the bus? Maybe there's a reason I am missing. If we can capture this on the main thread by letting the bus do its jobs we can avoid this whole function and the whole mess of deferring things through the main loop with that cascade of functions.

If this can't be done by any reason, I still recommend to reinject an application message though the bus and capturing it at the async handler.

If this doesn't work for some reason, I'd recommend to use more lambdas to ease the reading.

@calvaris

calvaris May 5, 2016

Collaborator

And now the expected explanation. Why do we capture this message here to defer it to the main thread instead of just letting it go through to the main thread through the bus? Maybe there's a reason I am missing. If we can capture this on the main thread by letting the bus do its jobs we can avoid this whole function and the whole mess of deferring things through the main loop with that cascade of functions.

If this can't be done by any reason, I still recommend to reinject an application message though the bus and capturing it at the async handler.

If this doesn't work for some reason, I'd recommend to use more lambdas to ease the reading.

This comment has been minimized.

@eocanha

eocanha May 30, 2016

Owner

handleSyncMessage() is usually meant to process messages on the playback pipeline but, as strange as it may seem, it can also receive messages from the AppendPipeline. After some tests, I found that the code to manage the duration via sync events isn't actually needed. Duration is managed directly by MediaSource, SourceBuffer and AppendPipeline in other ways, so I ended up removing all this code.

We also manage other sync events downstream, but by the moment these aren't needed for this backport. Less code, less worries.

@eocanha

eocanha May 30, 2016

Owner

handleSyncMessage() is usually meant to process messages on the playback pipeline but, as strange as it may seem, it can also receive messages from the AppendPipeline. After some tests, I found that the code to manage the duration via sync events isn't actually needed. Duration is managed directly by MediaSource, SourceBuffer and AppendPipeline in other ways, so I ended up removing all this code.

We also manage other sync events downstream, but by the moment these aren't needed for this backport. Less code, less worries.

This comment has been minimized.

@calvaris

calvaris May 31, 2016

Collaborator

Better to remove the code if it is not needed. If it were needed the solution would be to refactor that code as I don't think it is a good idea to mix messages from one pipeline to the other, etc.

@calvaris

calvaris May 31, 2016

Collaborator

Better to remove the code if it is not needed. If it were needed the solution would be to refactor that code as I don't think it is a good idea to mix messages from one pipeline to the other, etc.

@@ -744,6 +774,8 @@ void SourceBuffer::removeCodedFrames(const MediaTime& start, const MediaTime& en
DecodeOrderSampleMap::MapType erasedSamples(removeDecodeStart, removeDecodeEnd);
RefPtr<TimeRanges> erasedRanges = removeSamplesFromTrackBuffer(erasedSamples, trackBuffer, this, "removeCodedFrames");
+ // GStreamer backend doesn't support re-enqueue without preceding flushing seek, so just avoid adding same data to pipeline
+#if !USE(GSTREAMER)
// Only force the TrackBuffer to re-enqueue if the removed ranges overlap with enqueued and possibly
// not yet displayed samples.
if (currentMediaTime < trackBuffer.lastEnqueuedPresentationTime) {

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Make these comments consistent. Either both with long lines or both with short lines. I prefer longer lines, but former comment had shorter lines.

@calvaris

calvaris May 5, 2016

Collaborator

Make these comments consistent. Either both with long lines or both with short lines. I prefer longer lines, but former comment had shorter lines.

@@ -1534,6 +1578,8 @@ void SourceBuffer::sourceBufferPrivateDidReceiveSample(SourceBufferPrivate*, Pas
RefPtr<TimeRanges> erasedRanges = removeSamplesFromTrackBuffer(dependentSamples, trackBuffer, this, "sourceBufferPrivateDidReceiveSample");
+ // GStreamer backend doesn't support re-enqueue without preceding flushing seek, so just avoid adding same data to pipeline

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Ditto. Comments also begin with capitals and end with period.

@calvaris

calvaris May 5, 2016

Collaborator

Ditto. Comments also begin with capitals and end with period.

@@ -1543,7 +1589,7 @@ void SourceBuffer::sourceBufferPrivateDidReceiveSample(SourceBufferPrivate*, Pas
if (possiblyEnqueuedRanges.length())
trackBuffer.needsReenqueueing = true;
}
-

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Why is this lempty line disappearing?

@calvaris

calvaris May 5, 2016

Collaborator

Why is this lempty line disappearing?

@@ -1356,6 +1389,17 @@ void SourceBuffer::sourceBufferPrivateDidReceiveSample(SourceBufferPrivate*, Pas
it = m_trackBufferMap.add(trackID, TrackBuffer()).iterator;
TrackBuffer& trackBuffer = it->value;
+#if USE(GSTREAMER)
+ // FIXME: Hack to add fake range to fill start hole
+ double fakeRangeEnd = 0.0;

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Let's put it more neo-language. "Fake" sounds bad, let's use "gap" ;) And use only = 0;

@calvaris

calvaris May 5, 2016

Collaborator

Let's put it more neo-language. "Fake" sounds bad, let's use "gap" ;) And use only = 0;

@@ -22,6 +22,10 @@
#include <gst/video/video-format.h>
#include <gst/video/video-info.h>
+#define TRACE_MEDIA_MESSAGE(...) do { \
+ GST_TRACE(__VA_ARGS__); \
+ LOG_VERBOSE(Media, __VA_ARGS__); } while (0)

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

I love this cause I use the GStreamer log a lot.

Food for thought and to take into consideration in the future and/or in a separate patch.

Should we consider not logging these to WebKit as WK has only one log level and it could be flooding?

Should we consider for another bug reworking all these levels to prepend a string with the level? Something like: "Warning: ", "Trace: ", etc. That way it would be easier to grep them out.

@calvaris

calvaris May 5, 2016

Collaborator

I love this cause I use the GStreamer log a lot.

Food for thought and to take into consideration in the future and/or in a separate patch.

Should we consider not logging these to WebKit as WK has only one log level and it could be flooding?

Should we consider for another bug reworking all these levels to prepend a string with the level? Something like: "Warning: ", "Trace: ", etc. That way it would be easier to grep them out.

@@ -78,6 +78,7 @@ class MediaPlayerPrivateGStreamer : public MediaPlayerPrivateGStreamerBase {
#if ENABLE(MEDIA_SOURCE)
void load(const String& url, MediaSourcePrivateClient*) override;
#endif
+

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Leave this as it was.

@calvaris

calvaris May 5, 2016

Collaborator

Leave this as it was.

+/*
+ * Copyright (C) 2007, 2009 Apple Inc. All rights reserved.
+ * Copyright (C) 2007 Collabora Ltd. All rights reserved.
+ * Copyright (C) 2007 Alp Toker <alp@atoker.com>

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Is there any relevant code to keep these lines?

@calvaris

calvaris May 5, 2016

Collaborator

Is there any relevant code to keep these lines?

This comment has been minimized.

@eocanha

eocanha May 31, 2016

Owner

Yes. MediaPlayerPrivateGStreamerMSE was created by duplicating MediaPlayerPrivateGStreamer (affected by the above copyright notices), modifying what was needed to make MSE work, and then removing/refactoring the common parts with MediaPlayerPrivateGStreamer. I've checked that some of the lines of code added in the commits which introduced each of the above copyrights are still present in MediaPlayerPrivateGStreamerMSE.

@eocanha

eocanha May 31, 2016

Owner

Yes. MediaPlayerPrivateGStreamerMSE was created by duplicating MediaPlayerPrivateGStreamer (affected by the above copyright notices), modifying what was needed to make MSE work, and then removing/refactoring the common parts with MediaPlayerPrivateGStreamer. I've checked that some of the lines of code added in the commits which introduced each of the above copyrights are still present in MediaPlayerPrivateGStreamerMSE.

+
+ static bool isAvailable();
+
+ // TODO: reduce code duplication.

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

"Reduce", capital. TODO -> FIXME

@calvaris

calvaris May 5, 2016

Collaborator

"Reduce", capital. TODO -> FIXME

+ unsigned long droppedVideoFrames() override { return 0; }
+ unsigned long corruptedVideoFrames() override { return 0; }
+ MediaTime totalFrameDelay() override { return MediaTime::zeroTime(); }
+ bool timeIsBuffered(const MediaTime &time) const;

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

isTimeBuffered

@calvaris

calvaris May 5, 2016

Collaborator

isTimeBuffered

+ RefPtr<MediaSourceClientGStreamerMSE> m_mediaSourceClient;
+ MediaTime m_mediaTimeDuration;
+ mutable bool m_eosPending;
+ bool m_eosMarked;

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

In these two attribute clusters, you might want to alphabetically sort the attributes inside the cluster, unless you have another rule (importance, whatever)

@calvaris

calvaris May 5, 2016

Collaborator

In these two attribute clusters, you might want to alphabetically sort the attributes inside the cluster, unless you have another rule (importance, whatever)

+ // TODO: reduce code duplication.
+ void updateStates() override;
+
+ bool doSeek(gint64 position, float rate, GstSeekFlags seekType) override;

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Remove at least the seekType name from the signature. position and rate could remain for self-explanatory reasons but GstSeekFlags is self-explanatory enough to remove the attribute name.

@calvaris

calvaris May 5, 2016

Collaborator

Remove at least the seekType name from the signature. position and rate could remain for self-explanatory reasons but GstSeekFlags is self-explanatory enough to remove the attribute name.

+ void setMediaSourceClient(PassRefPtr<MediaSourceClientGStreamerMSE>);
+ RefPtr<MediaSourceClientGStreamerMSE> mediaSourceClient();
+
+ RefPtr<AppendPipeline> appendPipelineByTrackId(const AtomicString& trackId);

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Remove attribute name.

@calvaris

calvaris May 5, 2016

Collaborator

Remove attribute name.

+
+public:
+ static PassRefPtr<GStreamerMediaSample> create(GstSample* sample, const FloatSize& presentationSize, const AtomicString& trackID);
+ static PassRefPtr<GStreamerMediaSample> createFakeSample(GstCaps* caps, MediaTime pts, MediaTime dts, MediaTime duration, const FloatSize& presentationSize, const AtomicString& trackID);

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Remove unneeded attribute names.

@calvaris

calvaris May 5, 2016

Collaborator

Remove unneeded attribute names.

+
+ virtual ~GStreamerMediaSample();
+
+ void applyPtsOffset(MediaTime timestampOffset);

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Ditto.

@calvaris

calvaris May 5, 2016

Collaborator

Ditto.

+
+class MediaSourceClientGStreamerMSE: public RefCounted<MediaSourceClientGStreamerMSE> {
+ public:
+ static PassRefPtr<MediaSourceClientGStreamerMSE> create(MediaPlayerPrivateGStreamerMSE* playerPrivate);

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Parameter name

@calvaris

calvaris May 5, 2016

Collaborator

Parameter name

+ bool append(PassRefPtr<SourceBufferPrivateGStreamer>, const unsigned char*, unsigned);
+ void removedFromMediaSource(RefPtr<SourceBufferPrivateGStreamer>);
+ void flushAndEnqueueNonDisplayingSamples(Vector<RefPtr<MediaSample> > samples);
+ void enqueueSample(PassRefPtr<MediaSample> sample);

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Ditto.

@calvaris

calvaris May 5, 2016

Collaborator

Ditto.

+ void abort(PassRefPtr<SourceBufferPrivateGStreamer>);
+ bool append(PassRefPtr<SourceBufferPrivateGStreamer>, const unsigned char*, unsigned);
+ void removedFromMediaSource(RefPtr<SourceBufferPrivateGStreamer>);
+ void flushAndEnqueueNonDisplayingSamples(Vector<RefPtr<MediaSample> > samples);

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

I don't know if I like the "Displaying" gerund here, cause samples do not display anything, they are displayed themselves. "Displayable" or "Displayed"?

@calvaris

calvaris May 5, 2016

Collaborator

I don't know if I like the "Displaying" gerund here, cause samples do not display anything, they are displayed themselves. "Displayable" or "Displayed"?

This comment has been minimized.

@eocanha

eocanha May 31, 2016

Owner

This method, as well as enqueueSample(), is named after the methods of the same name in the SourceBufferPrivate interface. I think it's wise not to rename it.

@eocanha

eocanha May 31, 2016

Owner

This method, as well as enqueueSample(), is named after the methods of the same name in the SourceBufferPrivate interface. I think it's wise not to rename it.

+ // From our WebKitMediaSrc
+ // FIXME: these can be removed easily by directly invoking methods on SourceBufferPrivateGStreamer
+ void didReceiveInitializationSegment(SourceBufferPrivateGStreamer*, const SourceBufferPrivateClient::InitializationSegment&);
+ void didReceiveAllPendingSamples(SourceBufferPrivateGStreamer* sourceBuffer);

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Ditto. Fix comments (capitals and periods)

@calvaris

calvaris May 5, 2016

Collaborator

Ditto. Fix comments (capitals and periods)

+ GRefPtr<WebKitMediaSrc> webKitMediaSrc();
+
+ private:
+ MediaSourceClientGStreamerMSE(MediaPlayerPrivateGStreamerMSE* playerPrivate);

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Ditto.

@calvaris

calvaris May 5, 2016

Collaborator

Ditto.

#include <wtf/glib/GRefPtr.h>
+#include <wtf/PassRefPtr.h>
+#include "NotImplemented.h"
+#include "TimeRanges.h"

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

Let's fix these headers. "NotImplemented.h" is duplicated and TimeRanges.h should go before WTF because WTF comes with <> and the others come with ""

@calvaris

calvaris May 5, 2016

Collaborator

Let's fix these headers. "NotImplemented.h" is duplicated and TimeRanges.h should go before WTF because WTF comes with <> and the others come with ""

}
MediaSourceGStreamer::~MediaSourceGStreamer()
{
+ for (HashSet<SourceBufferPrivateGStreamer*>::iterator it = m_sourceBuffers.begin(), end = m_sourceBuffers.end(); it != end; ++it)

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

it is shortened, might want to use "current" if you can't use "iterator".

@calvaris

calvaris May 5, 2016

Collaborator

it is shortened, might want to use "current" if you can't use "iterator".

+ // parameters.isMediaSource = true;
+ // parameters.type = contentType.type();
+ // parameters.codecs = contentType.parameter(ASCIILiteral("codecs"));
+ // if (MediaPlayerPrivateGStreamer::supportsType(parameters) == MediaPlayer::IsNotSupported)

This comment has been minimized.

@calvaris

calvaris May 5, 2016

Collaborator

We have VCS for this, you can remove it.

@calvaris

calvaris May 5, 2016

Collaborator

We have VCS for this, you can remove it.

+void webkit_media_src_set_mediaplayerprivate(WebKitMediaSrc* src, WebCore::MediaPlayerPrivateGStreamerMSE* mediaPlayerPrivate)
+{
+ GST_OBJECT_LOCK(src);
+ // Set to 0 on MediaPlayerPrivateGStreamer destruction, never a dangling pointer

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

0 -> nullptr and period.

@calvaris

calvaris May 9, 2016

Collaborator

0 -> nullptr and period.

@@ -51,10 +61,20 @@ struct _WebKitMediaSrc {
struct _WebKitMediaSrcClass {
GstBinClass parentClass;
+
+ /* notify app that number of audio/video/text streams changed */

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

//, capital and period

@calvaris

calvaris May 9, 2016

Collaborator

//, capital and period

+ * Copyright (C) 2007, 2009 Apple Inc. All rights reserved.
+ * Copyright (C) 2007 Collabora Ltd. All rights reserved.
+ * Copyright (C) 2007 Alp Toker <alp@atoker.com>
+ * Copyright (C) 2009 Gustavo Noronha Silva <gns@gnome.org>

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Should we keep all these people here?

@calvaris

calvaris May 9, 2016

Collaborator

Should we keep all these people here?

This comment has been minimized.

@eocanha

eocanha Jun 16, 2016

Owner

I've removed Cable Television Laboratories because the MPEG TS part they added to MediaPlayerPrivateGStreamer isn't present anymore in the MSE subclass. The rest of the people is justified, because the MSE subclass is a copypaste+simplification of MediaPlayerPrivateGStreamer.

@eocanha

eocanha Jun 16, 2016

Owner

I've removed Cable Television Laboratories because the MPEG TS part they added to MediaPlayerPrivateGStreamer isn't present anymore in the MSE subclass. The rest of the people is justified, because the MSE subclass is a copypaste+simplification of MediaPlayerPrivateGStreamer.

+ GMutex m_newSampleMutex;
+ GCond m_newSampleCondition;
+ GMutex m_padAddRemoveMutex;
+ GCond m_padAddRemoveCondition;

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Let's switch to WTF::Lock, WTF::Contidition and WTF::Locker (for scope locking)

@calvaris

calvaris May 9, 2016

Collaborator

Let's switch to WTF::Lock, WTF::Contidition and WTF::Locker (for scope locking)

+ GCond m_padAddRemoveCondition;
+
+ GstCaps* m_appSinkCaps;
+ GstCaps* m_demuxerSrcPadCaps;

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Should these be GRefPtr?

@calvaris

calvaris May 9, 2016

Collaborator

Should these be GRefPtr?

This comment has been minimized.

@eocanha

eocanha Jun 16, 2016

Owner

They are now, except for m_appsinkCaps, whose ownership is managed in a special way using gst_caps_replace().

@eocanha

eocanha Jun 16, 2016

Owner

They are now, except for m_appsinkCaps, whose ownership is managed in a special way using gst_caps_replace().

This comment has been minimized.

@calvaris

calvaris Jun 17, 2016

Collaborator

We discussed about m_appsinkCaps above.

@calvaris

calvaris Jun 17, 2016

Collaborator

We discussed about m_appsinkCaps above.

+
+void MediaPlayerPrivateGStreamerMSE::notifySeekNeedsData(const MediaTime& seekTime)
+{
+ // Reenqueue samples needed to resume playback in the new position

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Period

@calvaris

calvaris May 9, 2016

Collaborator

Period

+ // Use doSeek() instead. If anybody is calling this version of doSeek(), something is wrong.
+ notImplemented();
+ ASSERT_NOT_REACHED();
+ return false;

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

This feels weird. I wouldn't have both notImplemented and the assertion here considering that it would be a programming mistake. I'd leave only the assertion to begin with and they I'd try to force a compilation error somehow, maybe with a #define that leads to an #error or something. But anyway, it feels weird that we end up having this code. Could you please explain it?

@calvaris

calvaris May 9, 2016

Collaborator

This feels weird. I wouldn't have both notImplemented and the assertion here considering that it would be a programming mistake. I'd leave only the assertion to begin with and they I'd try to force a compilation error somehow, maybe with a #define that leads to an #error or something. But anyway, it feels weird that we end up having this code. Could you please explain it?

This comment has been minimized.

@eocanha

eocanha Jun 16, 2016

Owner

This is the signature of the method coming from the MediaPlayerPrivateGStreamer superclass. However, we're not using this signature to handle our seek requests, but a different one without parameters, plus the value stored in some private variables.

We manage seeks ourselves and we don't want any future change in the superclass to screw this behaviour, so if anybody calls the "old" method by mistake we want to notice it.

I don't see how the #define scheme that you propose could actually work in an overridden method like seek(X,Y,Z). The compiler would compile code using the superclass signature and superclass users wouldn't have any way to notice the wrong usage if they're just using the superclass.

@eocanha

eocanha Jun 16, 2016

Owner

This is the signature of the method coming from the MediaPlayerPrivateGStreamer superclass. However, we're not using this signature to handle our seek requests, but a different one without parameters, plus the value stored in some private variables.

We manage seeks ourselves and we don't want any future change in the superclass to screw this behaviour, so if anybody calls the "old" method by mistake we want to notice it.

I don't see how the #define scheme that you propose could actually work in an overridden method like seek(X,Y,Z). The compiler would compile code using the superclass signature and superclass users wouldn't have any way to notice the wrong usage if they're just using the superclass.

This comment has been minimized.

@calvaris

calvaris Jun 17, 2016

Collaborator

Ok, I understand. Anyway, I think it is better to remove the notImplemented because the method is actually implemented to raise an error ;)

@calvaris

calvaris Jun 17, 2016

Collaborator

Ok, I understand. Anyway, I think it is better to remove the notImplemented because the method is actually implemented to raise an error ;)

+ double rate = m_player->rate();
+ GstSeekFlags seekType = static_cast<GstSeekFlags>(GST_SEEK_FLAG_FLUSH | GST_SEEK_FLAG_ACCURATE);
+
+ // Always move to seeking state to report correct 'currentTime' while pending for actual seek to complete

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Period

@calvaris

calvaris May 9, 2016

Collaborator

Period

+ // Always move to seeking state to report correct 'currentTime' while pending for actual seek to complete
+ m_seeking = true;
+
+ // Check if playback pipeline is ready for seek

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Period

@calvaris

calvaris May 9, 2016

Collaborator

Period

+ return m_seeking;
+ }
+
+ // Stop accepting new samples until actual seek is finished

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Period

@calvaris

calvaris May 9, 2016

Collaborator

Period

+ // Stop accepting new samples until actual seek is finished
+ webkit_media_src_set_readyforsamples(WEBKIT_MEDIA_SRC(m_source.get()), false);
+
+ // Correct seek time if it helps to fix a small gap

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Period

@calvaris

calvaris May 9, 2016

Collaborator

Period

+
+ // Correct seek time if it helps to fix a small gap
+ if (!timeIsBuffered(seekTime)) {
+ // Look if a near future time (<0.1 sec.) is buffered and change the seek target time

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Period.

@calvaris

calvaris May 9, 2016

Collaborator

Period.

+ }
+ }
+
+ // Check if MSE has samples for requested time and defer actual seek if needed

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Period.

@calvaris

calvaris May 9, 2016

Collaborator

Period.

+ return true;
+ }
+
+ // Complete previous MSE seek if needed

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Period.

@calvaris

calvaris May 9, 2016

Collaborator

Period.

+ if (!m_mseSeekCompleted) {
+ m_mediaSource->monitorSourceBuffers();
+ ASSERT(m_mseSeekCompleted);
+ return m_seeking; // Note seekCompleted will recursively call us

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Period.

@calvaris

calvaris May 9, 2016

Collaborator

Period.

+ return false;
+ }
+
+ // The samples will be enqueued in notifySeekNeedsData()

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Period.

@calvaris

calvaris May 9, 2016

Collaborator

Period.

+ break;
+ case GST_STATE_CHANGE_FAILURE:
+ LOG_MEDIA_MESSAGE("Failure: State: %s, pending: %s", gst_element_state_get_name(state), gst_element_state_get_name(pending));
+ // Change failed

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Period

@calvaris

calvaris May 9, 2016

Collaborator

Period

+ LOG_MEDIA_MESSAGE("trackId is empty");
+ }
+
+ ASSERT(!(trackId.isNull() || trackId.isEmpty()));

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Doesn't isEmpty include isNull() use case?

@calvaris

calvaris May 9, 2016

Collaborator

Doesn't isEmpty include isNull() use case?

+ return result;
+ }
+
+ // spec says we should not return "probably" if the codecs string is empty

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Capital and period.

Btw, this might be what is making EME test 5 break, but in this case it would be because this is not working or something. Anyway, for tip, YouTube only accepts probably so we would be fixing something today and screwing it tomorrow.

@calvaris

calvaris May 9, 2016

Collaborator

Capital and period.

Btw, this might be what is making EME test 5 break, but in this case it would be because this is not working or something. Anyway, for tip, YouTube only accepts probably so we would be fixing something today and screwing it tomorrow.

+
+ AtomicString codec() const override
+ {
+ gchar* description = gst_pb_utils_get_codec_description(m_caps);

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

GUniquePtr < gchar >

@calvaris

calvaris May 9, 2016

Collaborator

GUniquePtr < gchar >

+ static void registerMediaEngine(MediaEngineRegistrar);
+
+ void load(const String &url) override;
+ void load(const String& url, MediaSourcePrivateClient*) override;

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Parameter names

@calvaris

calvaris May 9, 2016

Collaborator

Parameter names

This comment has been minimized.

@philn

philn May 10, 2016

There's no need to name parameters if the type is explicit enough. Names are needed only for standard types like int, String, etc.

@philn

philn May 10, 2016

There's no need to name parameters if the type is explicit enough. Names are needed only for standard types like int, String, etc.

This comment has been minimized.

@eocanha

eocanha Jun 16, 2016

Owner

Yes Phil, in this case I think they're needed. At least they appear in the MediaPlayerPrivateGStreamer superclass and in the MediaPlayerPrivateInterface. That has to mean something about how important is to keep the parameter name.

@eocanha

eocanha Jun 16, 2016

Owner

Yes Phil, in this case I think they're needed. At least they appear in the MediaPlayerPrivateGStreamer superclass and in the MediaPlayerPrivateInterface. That has to mean something about how important is to keep the parameter name.

This comment has been minimized.

@calvaris

calvaris Jun 17, 2016

Collaborator

Not necessarily, maybe it was just a too quick review at the moment. And let me use your argument against you, as the arguments are already in the superclass, it is less necessary to have them here ;)

@calvaris

calvaris Jun 17, 2016

Collaborator

Not necessarily, maybe it was just a too quick review at the moment. And let me use your argument against you, as the arguments are already in the superclass, it is less necessary to have them here ;)

This comment has been minimized.

@eocanha

eocanha Jun 17, 2016

Owner

Fair enough. I'm removing the parameter name.

@eocanha

eocanha Jun 17, 2016

Owner

Fair enough. I'm removing the parameter name.

+ MediaTime presentationTime() const { return m_pts; }
+ MediaTime decodeTime() const { return m_dts; }
+ MediaTime duration() const { return m_duration; }
+ AtomicString trackID() const { return m_trackID; }

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

trackId

@calvaris

calvaris May 9, 2016

Collaborator

trackId

This comment has been minimized.

@eocanha

eocanha Jun 16, 2016

Owner

Yes for m_trackID. No for the trackID() method name, which is inherited from the MediaSample superclass.

@eocanha

eocanha Jun 16, 2016

Owner

Yes for m_trackID. No for the trackID() method name, which is inherited from the MediaSample superclass.

+ MediaTime m_pts, m_dts, m_duration;
+ AtomicString m_trackID;
+ size_t m_size;
+ GstSample* m_sample;

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

What is going on with this sample? Just a pointer? Don't we need to keep a reference or anything? GRefPtr?

@calvaris

calvaris May 9, 2016

Collaborator

What is going on with this sample? Just a pointer? Don't we need to keep a reference or anything? GRefPtr?

+ RefPtr<AppendPipeline> ap() { return m_ap; }
+
+private:
+ GstSample* m_sample;

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

GRefPtr?

@calvaris

calvaris May 9, 2016

Collaborator

GRefPtr?

+ RefPtr<AppendPipeline> ap() { return m_ap; }
+
+private:
+ GstPad* m_demuxerSrcPad;

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

GRefPtr?

@calvaris

calvaris May 9, 2016

Collaborator

GRefPtr?

This comment has been minimized.

@eocanha

eocanha Jun 16, 2016

Owner

PadInfo doesn't exist anymore.

@eocanha

eocanha Jun 16, 2016

Owner

PadInfo doesn't exist anymore.

+
+ LOG_MEDIA_MESSAGE("%p", this);
+
+ // TODO: give a name to the pipeline, maybe related with the track it's managing.

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

FIXME

@calvaris

calvaris May 9, 2016

Collaborator

FIXME

+
+ LOG_MEDIA_MESSAGE("%p", this);
+
+ // TODO: Maybe notify appendComplete here?

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

FIXME

@calvaris

calvaris May 9, 2016

Collaborator

FIXME

+
+ if (m_demuxerSrcPadCaps)
+ gst_caps_unref(m_demuxerSrcPadCaps);
+ m_demuxerSrcPadCaps = demuxerSrcPadCaps;

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

I guess this code should be managed by GRefPtr

@calvaris

calvaris May 9, 2016

Collaborator

I guess this code should be managed by GRefPtr

+ return;
+
+ GRefPtr<GstPad> pad = adoptGRef(gst_element_get_static_pad(m_appsink, "sink"));
+ GstCaps* caps = gst_pad_get_current_caps(pad.get());

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

GRefPtr?

@calvaris

calvaris May 9, 2016

Collaborator

GRefPtr?

This comment has been minimized.

@eocanha

eocanha Jun 16, 2016

Owner

No, because of gst_caps_replace().

@eocanha

eocanha Jun 16, 2016

Owner

No, because of gst_caps_replace().

This comment has been minimized.

@calvaris

calvaris Jun 17, 2016

Collaborator

Discussed above.

@calvaris

calvaris Jun 17, 2016

Collaborator

Discussed above.

+ g_mutex_lock(&m_newSampleMutex);
+
+ // Ignore samples if we're not expecting them. Refuse processing if we're in Invalid state.
+ if (!(m_appendStage == Ongoing || m_appendStage == Sampling)) {

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Apply De Morgan here, please

@calvaris

calvaris May 9, 2016

Collaborator

Apply De Morgan here, please

+ return;
+ }
+
+ // Add a fake sample if a gap is detected before the first sample

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Period.

@calvaris

calvaris May 9, 2016

Collaborator

Period.

+ g_mutex_unlock(&m_newSampleMutex);
+
+ {
+ static int i = 0;

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Add a comment here:

// This is here for debugging purposes. It does not pay off to have it as class member.

@calvaris

calvaris May 9, 2016

Collaborator

Add a comment here:

// This is here for debugging purposes. It does not pay off to have it as class member.

+ ASSERT(WTF::isMainThread());
+ LOG_MEDIA_MESSAGE("Connecting to appsink");
+
+ WTF::GMutexLocker<GMutex> lock(m_padAddRemoveMutex);

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

I hope this has already becode a WTF::Lock and therefore we use WTF::Locker here

@calvaris

calvaris May 9, 2016

Collaborator

I hope this has already becode a WTF::Lock and therefore we use WTF::Locker here

+
+#if !LOG_DISABLED
+ {
+ gchar* strcaps = gst_caps_to_string(caps.get());

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

GUniquePtr < gchar >

@calvaris

calvaris May 9, 2016

Collaborator

GUniquePtr < gchar >

+ m_track = WebCore::InbandTextTrackPrivateGStreamer::create(id(), sinkSinkPad.get());
+ break;
+ default:
+ // No useful data, but notify anyway to complete the append operation

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Period

@calvaris

calvaris May 9, 2016

Collaborator

Period

+ ASSERT(WTF::isMainThread());
+
+ // return adoptRef(new MediaSourceClientGStreamerMSE(playerPrivate));
+ // No adoptRef because the ownership has already been transferred to MediaPlayerPrivateGStreamerMSE

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Period.

@calvaris

calvaris May 9, 2016

Collaborator

Period.

+{
+ ASSERT(WTF::isMainThread());
+
+ // return adoptRef(new MediaSourceClientGStreamerMSE(playerPrivate));

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

Remove

@calvaris

calvaris May 9, 2016

Collaborator

Remove

+ ASSERT(WTF::isMainThread());
+
+ if (!m_playerPrivate)
+ return GRefPtr<WebKitMediaSrc>(NULL);

This comment has been minimized.

@calvaris

calvaris May 9, 2016

Collaborator

NULL -> nullptr

@calvaris

calvaris May 9, 2016

Collaborator

NULL -> nullptr

}
-// TODO: Implement these
-void SourceBufferPrivateGStreamer::flushAndEnqueueNonDisplayingSamples(Vector<RefPtr<MediaSample>>, AtomicString)
+void SourceBufferPrivateGStreamer::flushAndEnqueueNonDisplayingSamples(Vector<RefPtr<MediaSample> > samples, AtomicString trackIDString)

This comment has been minimized.

@philn

philn May 10, 2016

No need for a space between the > chars. This goes for all the patch :)

@philn

philn May 10, 2016

No need for a space between the > chars. This goes for all the patch :)

eocanha pushed a commit that referenced this pull request Dec 2, 2016

caitp@igalia.com