Changeset 254200 in webkit

Timestamp:

Jan 8, 2020 7:57:03 AM (4 years ago)

Author:

aboya@igalia.com

Message:

[WTF] Allow MediaTime static constants
​https://bugs.webkit.org/show_bug.cgi?id=205723

Reviewed by Darin Adler.

Source/WebCore:

Since MediaTime no longer has a non-trivial destructor, declaring
MediaTime variables no longer has potential side effects as far as the
compiler is concerned and it can therefore print "unused variable"
errors where that is the case.

This patch marks one of such variable as intentionally unused, since
it's only used in Debug and would otherwise break the Release build.
Actually unused variables are also removed.

• platform/graphics/cocoa/WebCoreDecompressionSession.mm:

(WebCore::WebCoreDecompressionSession::imageForTime):

• Modules/mediasource/SourceBuffer.cpp:

(WebCore::SourceBuffer::sourceBufferPrivateFastSeekTimeForMediaTime):

• Modules/mediasource/SourceBuffer.cpp:

(WebCore::SourceBuffer::removeCodedFrames):

Source/WTF:

Despite all its convenience methods, at its core MediaTime is a rather
trivial class with only integral members. Despite this, since it had a
destructor declared, this made the class non-trivially destructible
even if the implementation was empty, and therefore clang did not
allow to use it for static variables unless done in form of a pointer.

By removing the destructor this restriction is lifted and we don't
need heap allocations for static MediaTime objects.

Previous usages of heap allocation for static MediaTime objects have
been rewritten to take advantage of this. Test coverage is provided by
successful compilation without [-Werror,-Wexit-time-destructors]
errors and existing tests.

• wtf/MediaTime.cpp:

(WTF::MediaTime::zeroTime):
(WTF::MediaTime::invalidTime):
(WTF::MediaTime::positiveInfiniteTime):
(WTF::MediaTime::negativeInfiniteTime):
(WTF::MediaTime::indefiniteTime):
(WTF::MediaTime::~MediaTime): Deleted.

• wtf/MediaTime.h:

Location:

trunk/Source

Files:

• WTF/ChangeLog (modified) (1 diff)
• WTF/wtf/MediaTime.cpp (modified) (3 diffs)
• WTF/wtf/MediaTime.h (modified) (1 diff)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/Modules/mediasource/SourceBuffer.cpp (modified) (2 diffs)
• WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm (modified) (1 diff)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2020-01-08  Alicia Boya García  <aboya@igalia.com>
+
+        [WTF] Allow MediaTime static constants
+        https://bugs.webkit.org/show_bug.cgi?id=205723
+
+        Reviewed by Darin Adler.
+
+        Despite all its convenience methods, at its core MediaTime is a rather
+        trivial class with only integral members. Despite this, since it had a
+        destructor declared, this made the class non-trivially destructible
+        even if the implementation was empty, and therefore clang did not
+        allow to use it for static variables unless done in form of a pointer.
+
+        By removing the destructor this restriction is lifted and we don't
+        need heap allocations for static MediaTime objects.
+
+        Previous usages of heap allocation for static MediaTime objects have
+        been rewritten to take advantage of this. Test coverage is provided by
+        successful compilation without [-Werror,-Wexit-time-destructors]
+        errors and existing tests.
+
+        * wtf/MediaTime.cpp:
+        (WTF::MediaTime::zeroTime):
+        (WTF::MediaTime::invalidTime):
+        (WTF::MediaTime::positiveInfiniteTime):
+        (WTF::MediaTime::negativeInfiniteTime):
+        (WTF::MediaTime::indefiniteTime):
+        (WTF::MediaTime::~MediaTime): Deleted.
+        * wtf/MediaTime.h:
+
 2020-01-07  Said Abou-Hallawa  <sabouhallawa@apple.com>
 

trunk/Source/WTF/wtf/MediaTime.cpp

@@ -42 +42 @@
 namespace WTF {
 
+static_assert(std::is_trivially_destructible_v<MediaTime>, "MediaTime should be trivially destructible.");
+
 static uint32_t greatestCommonDivisor(uint32_t a, uint32_t b)
 {
@@ -87 +89 @@
 
     *this = value < 0 ? negativeInfiniteTime() : positiveInfiniteTime();
-}
-
-MediaTime::~MediaTime()
-{
 }
 
@@ -458 +456 @@
 const MediaTime& MediaTime::zeroTime()
 {
-    static const MediaTime* time = new MediaTime(0, 1, Valid);
-    return *time;
+    static const MediaTime time(0, 1, Valid);
+    return time;
 }
 
 const MediaTime& MediaTime::invalidTime()
 {
-    static const MediaTime* time = new MediaTime(-1, 1, 0);
-    return *time;
+    static const MediaTime time(-1, 1, 0);
+    return time;
 }
 
 const MediaTime& MediaTime::positiveInfiniteTime()
 {
-    static const MediaTime* time = new MediaTime(0, 1, PositiveInfinite | Valid);
-    return *time;
+    static const MediaTime time(0, 1, PositiveInfinite | Valid);
+    return time;
 }
 
 const MediaTime& MediaTime::negativeInfiniteTime()
 {
-    static const MediaTime* time = new MediaTime(-1, 1, NegativeInfinite | Valid);
-    return *time;
+    static const MediaTime time(-1, 1, NegativeInfinite | Valid);
+    return time;
 }
 
 const MediaTime& MediaTime::indefiniteTime()
 {
-    static const MediaTime* time = new MediaTime(0, 1, Indefinite | Valid);
-    return *time;
+    static const MediaTime time(0, 1, Indefinite | Valid);
+    return time;
 }
 

trunk/Source/WTF/wtf/MediaTime.h

@@ -57 +57 @@
     MediaTime(int64_t value, uint32_t scale, uint8_t flags = Valid);
     MediaTime(const MediaTime& rhs);
-    ~MediaTime();
 
     static MediaTime createWithFloat(float floatTime);

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-01-08  Alicia Boya García  <aboya@igalia.com>
+
+        [WTF] Allow MediaTime static constants
+        https://bugs.webkit.org/show_bug.cgi?id=205723
+
+        Reviewed by Darin Adler.
+
+        Since MediaTime no longer has a non-trivial destructor, declaring
+        MediaTime variables no longer has potential side effects as far as the
+        compiler is concerned and it can therefore print "unused variable"
+        errors where that is the case.
+
+        This patch marks one of such variable as intentionally unused, since
+        it's only used in Debug and would otherwise break the Release build.
+        Actually unused variables are also removed.
+
+        * platform/graphics/cocoa/WebCoreDecompressionSession.mm:
+        (WebCore::WebCoreDecompressionSession::imageForTime):
+        * Modules/mediasource/SourceBuffer.cpp:
+        (WebCore::SourceBuffer::sourceBufferPrivateFastSeekTimeForMediaTime):
+        * Modules/mediasource/SourceBuffer.cpp:
+        (WebCore::SourceBuffer::removeCodedFrames):
+
 2020-01-08  Diego Pino Garcia  <dpino@igalia.com>
 

trunk/Source/WebCore/Modules/mediasource/SourceBuffer.cpp

@@ -496 +496 @@
 {
     MediaTime seekTime = targetTime;
-    MediaTime lowerBoundTime = targetTime - negativeThreshold;
-    MediaTime upperBoundTime = targetTime + positiveThreshold;
 
     for (auto& trackBuffer : m_trackBufferMap.values()) {
@@ -811 +809 @@
 
     // 1. Let start be the starting presentation timestamp for the removal range.
-    MediaTime durationMediaTime = m_source->duration();
     MediaTime currentMediaTime = m_source->currentTime();
 

trunk/Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.mm

@@ -475 +475 @@
 
     MediaTime startTime = PAL::toMediaTime(CMBufferQueueGetFirstPresentationTimeStamp(m_producerQueue.get()));
+#if !LOG_DISABLED
     MediaTime endTime = PAL::toMediaTime(CMBufferQueueGetEndPresentationTimeStamp(m_producerQueue.get()));
+#endif
     if (!allowLater && time < startTime) {
         LOG(Media, "WebCoreDecompressionSession::imageForTime(%p) - time(%s) too early for queue(%s -> %s)", this, toString(time).utf8().data(), toString(startTime).utf8().data(), toString(endTime).utf8().data());