[Bug 740196] baseparse: Add support to skip padding areas in segments

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=740196

Tim-Philipp Müller <t.i.m at zen.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #300235|none                        |needs-work
             status|                            |

--- Comment #6 from Tim-Philipp Müller <t.i.m at zen.co.uk> ---
Comment on attachment 300235
  --> https://bugzilla.gnome.org/attachment.cgi?id=300235
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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.