Subtitle set position redesigned. Fix subtitle position reporting (#354)

Summary: Subtitle set position redesigned. Fix subtitle position
reporting
Type: Fix
Test Plan: UT/ CT, Fullstack
Jira: RIALTO-705

media/server/gstplayer/include/GstGenericPlayer.h

@@ -307,6 +307,13 @@ class GstGenericPlayer : public IGstGenericPlayer, public IGstGenericPlayerPriva
     std::optional<firebolt::rialto::wrappers::AudioAttributesPrivate>
     createAudioAttributes(const std::unique_ptr<IMediaPipeline::MediaSource> &source) const;
 
+    /**
+     * @brief Sets text track position before pushing data
+     *
+     * @param[in] source : the subtitle media source
+     */
+    void setTextTrackPositionIfRequired(GstElement *source);
+
 private:
     /**
      * @brief The player context.

media/server/gstplayer/include/GstRialtoTextTrackSinkPrivate.h

@@ -23,6 +23,8 @@
 #include "ITextTrackSession.h"
 #include <atomic>
 #include <memory>
+#include <mutex>
+#include <optional>
 #include <string>
 
 namespace firebolt::rialto::server
@@ -32,7 +34,9 @@ struct GstRialtoTextTrackSinkPrivate
     std::unique_ptr<ITextTrackSession> m_textTrackSession;
     std::atomic<bool> m_isMuted{false};
     std::string m_textTrackIdentifier;
-    // std::mutex m_mutex;
+    bool m_capsSet{false};
+    std::optional<uint64_t> m_queuedPosition;
+    std::mutex m_mutex;
 };
 }; // namespace firebolt::rialto::server
 

media/server/gstplayer/include/tasks/generic/SetSourcePosition.h

@@ -21,6 +21,7 @@
 #define FIREBOLT_RIALTO_SERVER_TASKS_GENERIC_SET_SOURCE_POSITION_H_
 
 #include "GenericPlayerContext.h"
+#include "IGlibWrapper.h"
 #include "IGstGenericPlayerClient.h"
 #include "IGstGenericPlayerPrivate.h"
 #include "IGstWrapper.h"
@@ -34,9 +35,8 @@ class SetSourcePosition : public IPlayerTask
 {
 public:
     SetSourcePosition(GenericPlayerContext &context, IGstGenericPlayerPrivate &player, IGstGenericPlayerClient *client,
-                      const std::shared_ptr<firebolt::rialto::wrappers::IGstWrapper> &gstWrapper,
-                      const MediaSourceType &type, std::int64_t position, bool resetTime, double appliedRate,
-                      uint64_t stopPosition);
+                      const std::shared_ptr<wrappers::IGlibWrapper> &glibWrapper, const MediaSourceType &type,
+                      std::int64_t position, bool resetTime, double appliedRate, uint64_t stopPosition);
     ~SetSourcePosition() override;
     void execute() const override;
 
@@ -45,7 +45,7 @@ class SetSourcePosition : public IPlayerTask
     GenericPlayerContext &m_context;
     IGstGenericPlayerPrivate &m_player;
     IGstGenericPlayerClient *m_gstPlayerClient;
-    std::shared_ptr<firebolt::rialto::wrappers::IGstWrapper> m_gstWrapper;
+    std::shared_ptr<wrappers::IGlibWrapper> m_glibWrapper;
     MediaSourceType m_type;
     std::int64_t m_position;
     bool m_resetTime;

media/server/gstplayer/source/GstGenericPlayer.cpp

@@ -765,7 +765,14 @@ void GstGenericPlayer::attachData(const firebolt::rialto::MediaSourceType mediaT
             return;
         }
 
-        pushSampleIfRequired(streamInfo.appSrc, common::convertMediaSourceType(mediaType));
+        if (firebolt::rialto::MediaSourceType::SUBTITLE == mediaType)
+        {
+            setTextTrackPositionIfRequired(streamInfo.appSrc);
+        }
+        else
+        {
+            pushSampleIfRequired(streamInfo.appSrc, common::convertMediaSourceType(mediaType));
+        }
         if (mediaType == firebolt::rialto::MediaSourceType::AUDIO)
         {
             // This needs to be done before gstAppSrcPushBuffer() is
@@ -950,6 +957,21 @@ void GstGenericPlayer::pushSampleIfRequired(GstElement *source, const std::strin
     return;
 }
 
+void GstGenericPlayer::setTextTrackPositionIfRequired(GstElement *source)
+{
+    auto initialPosition = m_context.initialPositions.find(source);
+    if (m_context.initialPositions.end() == initialPosition)
+    {
+        // Sending initial sample not needed
+        return;
+    }
+
+    m_glibWrapper->gObjectSet(m_context.subtitleSink, "position",
+                              static_cast<guint64>(initialPosition->second.back().position), nullptr);
+
+    m_context.initialPositions.erase(initialPosition);
+}
+
 bool GstGenericPlayer::reattachSource(const std::unique_ptr<IMediaPipeline::MediaSource> &source)
 {
     if (m_context.streamInfo.find(source->getType()) == m_context.streamInfo.end())

media/server/gstplayer/source/GstTextTrackSink.cpp

@@ -34,6 +34,7 @@ enum
     PROP_0,
     PROP_MUTE,
     PROP_TEXT_TRACK_IDENTIFIER,
+    PROP_POSITION,
     PROP_LAST
 };
 
@@ -85,6 +86,7 @@ static void gst_rialto_text_track_sink_get_property(GObject *object, guint propI
                                                     GValue *value, GParamSpec *pspec);
 static void gst_rialto_text_track_sink_set_property(GObject *object, guint propId, // NOLINT(build/function_format)
                                                     const GValue *value, GParamSpec *pspec);
+static gboolean gst_rialto_text_track_sink_query(GstElement *element, GstQuery *query); // NOLINT(build/function_format)
 
 static void gst_rialto_text_track_sink_class_init(GstRialtoTextTrackSinkClass *klass) // NOLINT(build/function_format)
 {
@@ -104,6 +106,7 @@ static void gst_rialto_text_track_sink_class_init(GstRialtoTextTrackSinkClass *k
     baseSinkClass->set_caps = GST_DEBUG_FUNCPTR(gst_rialto_text_track_sink_set_caps);
     baseSinkClass->event = GST_DEBUG_FUNCPTR(gst_rialto_text_track_sink_event);
     elementClass->change_state = GST_DEBUG_FUNCPTR(gst_rialto_text_track_sink_change_state);
+    elementClass->query = GST_DEBUG_FUNCPTR(gst_rialto_text_track_sink_query);
 
     g_object_class_install_property(gobjectClass, PROP_MUTE,
                                     g_param_spec_boolean("mute", "Mute", "Mute subtitles", FALSE, G_PARAM_READWRITE));
@@ -112,6 +115,9 @@ static void gst_rialto_text_track_sink_class_init(GstRialtoTextTrackSinkClass *k
                                     g_param_spec_string("text-track-identifier", "Text Track Identifier",
                                                         "Identifier of text track", nullptr,
                                                         GParamFlags(G_PARAM_READWRITE)));
+    g_object_class_install_property(gobjectClass, PROP_POSITION,
+                                    g_param_spec_uint64("position", "Position", "Position", 0, G_MAXUINT64, 0,
+                                                        GParamFlags(G_PARAM_READWRITE)));
 
     gst_element_class_add_pad_template(elementClass, gst_static_pad_template_get(&sinkTemplate));
     gst_element_class_set_static_metadata(elementClass, "Rialto TextTrack Sink", "Sink/Parser/Subtitle",
@@ -226,6 +232,14 @@ static gboolean gst_rialto_text_track_sink_set_caps(GstBaseSink *sink, GstCaps *
 
     textTrackSink->priv->m_textTrackSession->mute(textTrackSink->priv->m_isMuted);
 
+    std::unique_lock lock{textTrackSink->priv->m_mutex};
+    textTrackSink->priv->m_capsSet = true;
+    if (textTrackSink->priv->m_queuedPosition.has_value())
+    {
+        textTrackSink->priv->m_textTrackSession->setPosition(textTrackSink->priv->m_queuedPosition.value() / GST_MSECOND);
+        textTrackSink->priv->m_queuedPosition.reset();
+    }
+
     return TRUE;
 }
 
@@ -236,15 +250,6 @@ static gboolean gst_rialto_text_track_sink_event(GstBaseSink *sink, GstEvent *ev
 
     switch (GST_EVENT_TYPE(event))
     {
-    case GST_EVENT_SEGMENT:
-    {
-        const GstSegment *segment;
-        gst_event_parse_segment(event, &segment);
-
-        GST_DEBUG_OBJECT(textTrackSink, "setting position to %" GST_TIME_FORMAT, GST_TIME_ARGS(segment->start));
-        textTrackSink->priv->m_textTrackSession->setPosition(segment->start / GST_MSECOND);
-        break;
-    }
     case GST_EVENT_FLUSH_START:
     {
         break;
@@ -330,6 +335,12 @@ static void gst_rialto_text_track_sink_get_property(GObject *object, guint propI
         g_value_set_string(value, priv->m_textTrackIdentifier.c_str());
         break;
     }
+    case PROP_POSITION:
+    {
+        // Thunder ITextTrack does not provide getPosition API so we are unable to determine current position
+        g_value_set_uint64(value, GST_CLOCK_TIME_NONE);
+        break;
+    }
     default:
         G_OBJECT_WARN_INVALID_PROPERTY_ID(object, propId, pspec);
         break;
@@ -368,12 +379,56 @@ static void gst_rialto_text_track_sink_set_property(GObject *object, guint propI
 
         break;
     }
+    case PROP_POSITION:
+    {
+        guint64 position = g_value_get_uint64(value);
+        std::unique_lock lock{priv->m_mutex};
+        if (priv->m_textTrackSession && priv->m_capsSet)
+        {
+            priv->m_textTrackSession->setPosition(position / GST_MSECOND);
+        }
+        else
+        {
+            priv->m_queuedPosition = position;
+        }
+        break;
+    }
     default:
         G_OBJECT_WARN_INVALID_PROPERTY_ID(object, propId, pspec);
         break;
     }
 }
 
+static gboolean gst_rialto_text_track_sink_query(GstElement *element, GstQuery *query) // NOLINT(build/function_format)
+{
+    GstRialtoTextTrackSink *sink = GST_RIALTO_TEXT_TRACK_SINK(element);
+    GST_DEBUG_OBJECT(sink, "handling query '%s'", GST_QUERY_TYPE_NAME(query));
+    switch (GST_QUERY_TYPE(query))
+    {
+    case GST_QUERY_POSITION:
+    {
+        GstFormat fmt;
+        gst_query_parse_position(query, &fmt, NULL);
+        switch (fmt)
+        {
+        case GST_FORMAT_TIME:
+        {
+            // GST_CLOCK_TIME_NONE has to be returned here, because otherwise whole pipeline returns incorrect position
+            gst_query_set_position(query, fmt, GST_CLOCK_TIME_NONE);
+            break;
+        }
+        default:
+            break;
+        }
+        return TRUE;
+    }
+    default:
+        break;
+    }
+    GstElement *parent = GST_ELEMENT(&sink->parent);
+    return GST_ELEMENT_CLASS(parent_class)->query(parent, query);
+}
+
 namespace firebolt::rialto::server
 {
 std::shared_ptr<IGstTextTrackSinkFactory> IGstTextTrackSinkFactory::createFactory()

media/server/gstplayer/source/tasks/generic/GenericPlayerTaskFactory.cpp

@@ -301,7 +301,7 @@ GenericPlayerTaskFactory::createSetSourcePosition(GenericPlayerContext &context,
                                                   const firebolt::rialto::MediaSourceType &type, std::int64_t position,
                                                   bool resetTime, double appliedRate, uint64_t stopPosition) const
 {
-    return std::make_unique<tasks::generic::SetSourcePosition>(context, player, m_client, m_gstWrapper, type, position,
+    return std::make_unique<tasks::generic::SetSourcePosition>(context, player, m_client, m_glibWrapper, type, position,
                                                                resetTime, appliedRate, stopPosition);
 }
 

media/server/gstplayer/source/tasks/generic/SetSourcePosition.cpp

@@ -26,10 +26,10 @@ namespace firebolt::rialto::server::tasks::generic
 {
 SetSourcePosition::SetSourcePosition(GenericPlayerContext &context, IGstGenericPlayerPrivate &player,
                                      IGstGenericPlayerClient *client,
-                                     const std::shared_ptr<firebolt::rialto::wrappers::IGstWrapper> &gstWrapper,
+                                     const std::shared_ptr<wrappers::IGlibWrapper> &glibWrapper,
                                      const MediaSourceType &type, std::int64_t position, bool resetTime,
                                      double appliedRate, uint64_t stopPosition)
-    : m_context{context}, m_player(player), m_gstPlayerClient{client}, m_gstWrapper{gstWrapper}, m_type{type},
+    : m_context{context}, m_player(player), m_gstPlayerClient{client}, m_glibWrapper{glibWrapper}, m_type{type},
       m_position{position}, m_resetTime{resetTime}, m_appliedRate{appliedRate}, m_stopPosition{stopPosition}
 {
     RIALTO_SERVER_LOG_DEBUG("Constructing SetSourcePosition");
@@ -69,16 +69,13 @@ void SetSourcePosition::execute() const
         m_context.initialPositions[source].emplace_back(
             SegmentData{m_position, m_resetTime, m_appliedRate, m_stopPosition});
     }
+    else if (MediaSourceType::SUBTITLE == m_type)
+    {
+        setSubtitlePosition(source);
+    }
 
     if (m_context.setupSourceFinished)
     {
-        if (MediaSourceType::SUBTITLE == m_type)
-        {
-            // in case of subtitles, all data might be already in the sink and we might not get any data anymore,
-            // so send the new segment here and to not depend on any following buffers
-            setSubtitlePosition(source);
-        }
-
         // Reset Eos info
         m_context.endOfStreamInfo.erase(m_type);
         m_context.eosNotified = false;
@@ -91,25 +88,17 @@ void SetSourcePosition::execute() const
 
 void SetSourcePosition::setSubtitlePosition(GstElement *source) const
 {
-    GstSegment *segment{m_gstWrapper->gstSegmentNew()};
-    m_gstWrapper->gstSegmentInit(segment, GST_FORMAT_TIME);
-    if (!m_gstWrapper->gstSegmentDoSeek(segment, m_context.playbackRate, GST_FORMAT_TIME, GST_SEEK_FLAG_NONE,
-                                        GST_SEEK_TYPE_SET, m_position, GST_SEEK_TYPE_SET, GST_CLOCK_TIME_NONE, nullptr))
+    // in case of subtitles, all data might be already in the sink and we might not get any data anymore,
+    // so set position here and to not depend on any following buffers
+    if (m_context.setupSourceFinished)
     {
-        RIALTO_SERVER_LOG_WARN("Segment seek failed.");
-        m_gstWrapper->gstSegmentFree(segment);
-        return;
+        m_glibWrapper->gObjectSet(m_context.subtitleSink, "position", static_cast<guint64>(m_position), nullptr);
     }
-
-    if (!m_gstWrapper->gstPadSendEvent(m_gstWrapper->gstBaseSinkPad(m_context.subtitleSink),
-                                       m_gstWrapper->gstEventNewSegment(segment)))
+    else
     {
-        RIALTO_SERVER_LOG_INFO("Pad not ready to receive events. Postpone sending segment");
         m_context.initialPositions[source].emplace_back(
             SegmentData{m_position, m_resetTime, m_appliedRate, m_stopPosition});
     }
-
-    m_gstWrapper->gstSegmentFree(segment);
 }
 
 } // namespace firebolt::rialto::server::tasks::generic

tests/unittests/media/server/gstplayer/genericPlayer/GstGenericPlayerPrivateTest.cpp

@@ -943,6 +943,47 @@ TEST_F(GstGenericPlayerPrivateTest, undefinedStopPositionInSetSourcePosition)
     m_sut->attachData(firebolt::rialto::MediaSourceType::AUDIO);
 }
 
+TEST_F(GstGenericPlayerPrivateTest, shouldPushSubtitleBuffer)
+{
+    GstBuffer buffer{};
+    GstAppSrc subSrc{};
+    GstAppSrc videoSrc{};
+    modifyContext(
+        [&](GenericPlayerContext &context)
+        {
+            context.streamInfo[firebolt::rialto::MediaSourceType::SUBTITLE].buffers.emplace_back(&buffer);
+            context.streamInfo[firebolt::rialto::MediaSourceType::SUBTITLE].isDataNeeded = true;
+            context.streamInfo[firebolt::rialto::MediaSourceType::SUBTITLE].appSrc = GST_ELEMENT(&subSrc);
+            context.streamInfo[firebolt::rialto::MediaSourceType::VIDEO].appSrc = GST_ELEMENT(&videoSrc);
+        });
+    EXPECT_CALL(*m_gstWrapperMock, gstAppSrcPushBuffer(_, &buffer));
+    m_sut->attachData(firebolt::rialto::MediaSourceType::SUBTITLE);
+}
+
+TEST_F(GstGenericPlayerPrivateTest, shouldPushSubtitleBufferAndSetPosition)
+{
+    constexpr std::int64_t kPosition{124};
+    constexpr bool kDoNotResetTime{false};
+    constexpr double kAppliedRate{1.0};
+    constexpr uint64_t kStopPosition{3453425};
+    GstBuffer buffer{};
+    GstAppSrc subSrc{};
+    GstAppSrc videoSrc{};
+    modifyContext(
+        [&](GenericPlayerContext &context)
+        {
+            context.streamInfo[firebolt::rialto::MediaSourceType::SUBTITLE].buffers.emplace_back(&buffer);
+            context.streamInfo[firebolt::rialto::MediaSourceType::SUBTITLE].isDataNeeded = true;
+            context.streamInfo[firebolt::rialto::MediaSourceType::SUBTITLE].appSrc = GST_ELEMENT(&subSrc);
+            context.initialPositions[GST_ELEMENT(&subSrc)].emplace_back(
+                SegmentData{kPosition, kDoNotResetTime, kAppliedRate, kStopPosition});
+            context.streamInfo[firebolt::rialto::MediaSourceType::VIDEO].appSrc = GST_ELEMENT(&videoSrc);
+        });
+    EXPECT_CALL(*m_glibWrapperMock, gObjectSetStub(_, StrEq("position")));
+    EXPECT_CALL(*m_gstWrapperMock, gstAppSrcPushBuffer(_, &buffer));
+    m_sut->attachData(firebolt::rialto::MediaSourceType::SUBTITLE);
+}
+
 TEST_F(GstGenericPlayerPrivateTest, shouldCancelAudioUnderflowAndResume)
 {
     GstBuffer buffer{};

tests/unittests/media/server/gstplayer/genericPlayer/common/GenericTasksTestsBase.cpp

@@ -2976,38 +2976,15 @@ void GenericTasksTestsBase::shouldFlushVideoSrcSuccess()
 
 void GenericTasksTestsBase::shouldSetSubtitleSourcePosition()
 {
-    EXPECT_CALL(*testContext->m_gstWrapper, gstSegmentNew()).WillOnce(Return(&testContext->m_segment));
-    EXPECT_CALL(*testContext->m_gstWrapper, gstSegmentInit(&testContext->m_segment, GST_FORMAT_TIME));
-    EXPECT_CALL(*testContext->m_gstWrapper,
-                gstSegmentDoSeek(&testContext->m_segment, kRate, GST_FORMAT_TIME, GST_SEEK_FLAG_NONE, GST_SEEK_TYPE_SET,
-                                 kPosition, GST_SEEK_TYPE_SET, GST_CLOCK_TIME_NONE, nullptr))
-        .WillOnce(Return(true));
-    EXPECT_CALL(*testContext->m_gstWrapper, gstEventNewSegment(&testContext->m_segment))
-        .WillOnce(Return(&testContext->m_event));
-    EXPECT_CALL(*testContext->m_gstWrapper, gstBaseSinkPad(&testContext->m_textTrackSink))
-        .WillOnce(Return(&testContext->m_pad));
-    EXPECT_CALL(*testContext->m_gstWrapper, gstPadSendEvent(&testContext->m_pad, &testContext->m_event))
-        .WillOnce(Return(false));
-    EXPECT_CALL(*testContext->m_gstWrapper, gstSegmentFree(&testContext->m_segment));
-}
-
-void GenericTasksTestsBase::shouldFailToSetSubtitleSourcePosition()
-{
-    EXPECT_CALL(*testContext->m_gstWrapper, gstSegmentNew()).WillOnce(Return(&testContext->m_segment));
-    EXPECT_CALL(*testContext->m_gstWrapper, gstSegmentInit(&testContext->m_segment, GST_FORMAT_TIME));
-    EXPECT_CALL(*testContext->m_gstWrapper,
-                gstSegmentDoSeek(&testContext->m_segment, kRate, GST_FORMAT_TIME, GST_SEEK_FLAG_NONE, GST_SEEK_TYPE_SET,
-                                 kPosition, GST_SEEK_TYPE_SET, GST_CLOCK_TIME_NONE, nullptr))
-        .WillOnce(Return(false));
-    EXPECT_CALL(*testContext->m_gstWrapper, gstSegmentFree(&testContext->m_segment));
+    EXPECT_CALL(*testContext->m_glibWrapper, gObjectSetStub(&testContext->m_textTrackSink, StrEq("position")));
 }
 
 void GenericTasksTestsBase::triggerSetSourcePosition(firebolt::rialto::MediaSourceType sourceType)
 {
     firebolt::rialto::server::tasks::generic::SetSourcePosition task{testContext->m_context,
                                                                      testContext->m_gstPlayer,
                                                                      &testContext->m_gstPlayerClient,
-                                                                     testContext->m_gstWrapper,
+                                                                     testContext->m_glibWrapper,
                                                                      sourceType,
                                                                      kPosition,
                                                                      kResetTime,