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

Fix segment handling in interpipesrc #98

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

mariannasb
Copy link

It was missing something like what is done in appsrc->create(): https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/-/blob/master/gst-libs/gst/app/gstappsrc.c#L1676

Related to #96

Fix invalid PTS/DTS for the case where there are multiple listeners with different stream-sync settings: fx one with restart-ts and the other with passthrough-ts.
The restart-ts branch was invalidating the PTS and DTS which would also affect the other branch since the buffer wasn't ensured as writable.
Fix restart-ts for multiple listeners
Send all queued events with "timestamp < buffer timestamp" instead of just the 1st one before sending the buffer.

Only the 1st event of the event queue with "timestamp < buffer timestamp" was being send before the buffer.
But there could be several events in the queue that should sent before the buffer.

Fixes RidgeRun#96

The segment event was not being handled properly when using x264enc + mpegtsmux together with interpipesrc/sink.
Which was causing 2 segment to be delivered to the mpegtsmux:
1st one generated by the interpipesrc (starting from zero) because no segment existed.
2nd coming from the x264 encoder (correcting the 1000 hours offset added by the encoder).

The 1st segment resulted in all subsequent DTS being seen as invalid (backwards) by the muxer.

This happened because the event queue contained the following: stream-start, segment, tag, tag, tag.
All with timestamp 0.
But only the stream-start was sent, then the 1st buffer, then the segment.
Which caused the DTS issues.
Fix segment handling in interpipesrc
@michaelgruner michaelgruner changed the base branch from master to develop September 24, 2021 21:39
@michaelgruner
Copy link
Collaborator

Hey @mariannasb thank you for your contribution. @mmontero can you take care of this review?

@@ -671,6 +688,7 @@ gst_inter_pipe_src_push_buffer (GstInterPipeIListener * iface,
GST_TIME_ARGS (GST_BUFFER_PTS (buffer)));
} else if (GST_INTER_PIPE_SRC_RESTART_TIMESTAMP == src->stream_sync) {
/* Remove the incoming timestamp to be generated according this basetime */
buffer = gst_buffer_make_writable (buffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove this change from this pull request this corresponds to a different fix

Copy link
Author

Choose a reason for hiding this comment

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

Done

const GstSegment *segment = NULL;

gst_event_parse_segment (serial_event, &segment);
g_assert (segment != NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

for runtime errors, this project uses GST_ERROR_OBJECT instead of assertions

Copy link
Author

Choose a reason for hiding this comment

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

Done

Comment on lines +740 to +744
if (GST_EVENT_TIMESTAMP (event) > (srcbasetime - basetime))
GST_EVENT_TIMESTAMP (event) =
GST_EVENT_TIMESTAMP (event) - (srcbasetime - basetime);
else
GST_EVENT_TIMESTAMP (event) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove these changes from this pull request that correspond to a different fix

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Author

Choose a reason for hiding this comment

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

This fix is now in #103

@mariannasb
Copy link
Author

Have also rebased on top of develop

@mariannasb
Copy link
Author

I ended up having to create a new pull request as this was based on my master branch

#102

@mmontero mmontero self-requested a review October 7, 2021 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants