Review of attachment 290777 [details] [review]:

How would the subclass distinguish the "complete" segment, or a smaller segment caused by a seek (i.e. bigger start position, smaller stop position, or both). I think you would need that for the LAME tags as you only want to modify the very beginning and end, not some segment that is in the middle of the stream.

::: libs/gst/base/gstbaseparse.c
@@ +1245,3 @@
       ret = gst_pad_push_event (parse->srcpad, event);
     } else {
       // GST_VIDEO_DECODER_STREAM_LOCK (decoder);

Not your fault... but!

With LAME, I have the following checks in mpegaudioparse:

  /* if there is no LAME tag information, there is nothing to do */
  if ((mp3parse->encoder_delay == 0) && (mp3parse->encoder_padding == 0))
    return;

  /* only adjust segments which are open ended */
  if ((segment->start != 0) || (segment->stop != -1))
    return;

  /* only adjust TIME segments */
  if (segment->format != GST_FORMAT_TIME)
    return;

You are right that modifying any other segment type is not what you want in that case. I am not sure that limiting the adjust_segment function to open ended segments is wise though. Are there other use cases where you _do_ want to modify a segment that is in the middle of the stream for example?

Created attachment 298714 [details] [review]
Patch for new adjust_segment vfunc (updated for newest git master)

Comment on attachment 298714 [details] [review]
Patch for new adjust_segment vfunc (updated for newest git master)

I think this makes sense, can we get a unit test for that though?

Created attachment 300235 [details] [review]
Replacement patch for the vfunc approach; limited to padding adjustments

This is a new approach. Instead of adjusting the outgoing segments in a vfunc, add a function to specify padding times, which is what the vfunc would be used in virtually all cases anyway. Unlike the vfunc patch, this one does not cause problems during seeking.

Comment on attachment 300235 [details] [review]
Replacement patch for the vfunc approach; limited to padding adjustments

Not sure if it's a good idea to try and get this into 1.6 at this point.

What's not clear to me is that it should do these adjustments if the upstream segment has a notion of time, i.e. sends a TIME segment, because you're basically shifting the stream to trim off the initial samples at the start, which might cause some A/V desync, no?

On IRC you suggested it would be up to the subclass to decide when to set this or not, but I am not sure I agree with that. Minor issue of course.

>+/**
>+ * gst_base_parse_set_padding_times:
>+ * @parse: a #GstBaseParse
>+ * @start_padding: Amount of padding at the start of the segment
>+ * @end_padding: Amount of padding at the end of the segment

Should mention the units here (nanoseconds, not samples)

>+void
>+gst_base_parse_set_padding (GstBaseParse * parse, gint64 start_padding,
>+    gint64 end_padding)

Why are these not just GstClockTime? Do negative values ever make sense? If we just map them to 0 we might just as well require the subclass to pass 0 if no adjustment is needed.


>+static void
>+gst_base_parse_adjust_segment_in_event (GstBaseParse * parse, GstEvent ** event)

More of a style/flow issue: I don't like this function very much. I don't think it should ever be passed events other than SEGMENT events (I realise this makes code elsewhere less elegant, but it's not right like this imho), and ideally it should just operate on a GstSegment instead of an event. If we never adjust an upstream TIME segment that shouldn't make things more complicated because we generate the segment event ourselves then, from a GstSegment.


>+  /* No padding means nothing needs to be done here */
>+  if ((parse->priv->start_padding == 0) && (parse->priv->end_padding == 0))
>+    return;

General style issue everywhere: please use

  if (foo == 0 || bar == 0) 

without the additional brackets. It looks cleaner, and the compiler will warn if you accidentally type if ((foo = 0) || ...) (I know this is not always consistent in the gstreamer code base, still.)


>+  if (segment.stop == -1) {
>+    if ((segment.stop == -1) && (parse->priv->end_padding != 0)
>+        && (parse->priv->duration > 0)) {

Maybe check for segment.stop == -1 again, just to be sure :)

>+      segment.stop = parse->priv->duration - parse->priv->end_padding;

What if duration > end_padding ?

Is it safe to make up a segment.stop here, since the duration might be estimated and could still change later, no? I think it's only appropriate to do this before we push the last buffer (but then we'd have to keep back a buffer and push it on EOS after pushing an adjusted segment event...) (which would only be acceptable if input is non-live). Otherwise we might end up in a situation where the .stop causes an early stop/EOS.

I might be confused about this of course.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/82.