Review of attachment 276446 [details] [review]:

::: libs/gst/base/gstbaseparse.c
@@ +1998,3 @@
+      } else {
+        GST_DEBUG
+            ("This is more than available, flyshing %u, storing %u to skip", av,

Typo: flyshing :)

skip is an int in the strict (signed), but here you print the values with %u. It probably should all be unsigned everywhere? But then how do you distinguish "unset"==-1?

@@ +2745,3 @@
+    gsize bsize = gst_buffer_get_size (buffer);
+    GST_DEBUG ("Got %u buffer, need to skip %u", (unsigned) bsize,
+        (unsigned) parse->priv->skip);

G_GSIZE_FORMAT for gsize btw

@@ +2748,3 @@
+    if (parse->priv->skip >= bsize) {
+      parse->priv->skip -= bsize;
+      GST_DEBUG ("All the buffer is eaten");

s/eaten/skipped/ maybe?

@@ +2754,3 @@
+    }
+    buffer = gst_buffer_make_writable (buffer);
+    gst_buffer_resize (buffer, parse->priv->skip, bsize - parse->priv->skip);

It's cheaper to push into the adapter and then just flush the remaining skip bytes

@@ +2757,3 @@
+    parse->priv->offset += parse->priv->skip;
+    GST_DEBUG ("Done eating, we have %u left on this buffer",
+        (unsigned) gst_buffer_get_size (buffer));

gst_buffer_get_size() can be relatively expensive, also G_GSIZE_FORMAT

Created attachment 276753 [details] [review]
allow skipping more data than we currently have

All changed, except the gst_buffer_resize one. I'm not sure it's worth the extra code as it'd require remembering an extra skip amount as I can't add/flush here. And this is executed only for the last partial buffer. Do you think it's best to change it so anyway?

Review of attachment 276753 [details] [review]:

No, that's ok

::: libs/gst/base/gstbaseparse.c
@@ +355,3 @@
+
+  /* When we need to skip more data than we have currently */
+  gint skip;

This could be guint, right? You use 0 for unset anyway

@@ +1995,3 @@
+      /* If we're asked to skip more than is available in the adapter,
+         we need to remember what we need to skip for next iteration */
+      gint av = gst_adapter_available (parse->priv->adapter);

This returns a gsize

@@ +2001,3 @@
+      } else {
+        GST_DEBUG
+            ("This is more than available, flyshing %u, storing %u to skip", av,

%u is "unsigned int" aka guint. gsize would be G_GSIZE_FORMAT, for signed "int" use %d

@@ +2748,3 @@
+    gsize bsize = gst_buffer_get_size (buffer);
+    GST_DEBUG ("Got %u buffer, need to skip %u", (unsigned) bsize,
+        (unsigned) parse->priv->skip);

Same story here

@@ +2760,3 @@
+    parse->priv->offset += parse->priv->skip;
+    GST_DEBUG ("Done eating, we have %u left on this buffer",
+        (unsigned) gst_buffer_get_size (buffer));

And here

er, looks like I forgot to format-patch before reuploading so I put the same in, sorry :)

Created attachment 277713 [details] [review]
allow skipping more data than we have

I think all comments were addressed. I'll then push it in the near future if there are no more.

Pushed:

commit 7c4fbc9fb10d1381df766587303e9c0066613f42
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Tue May 13 11:18:08 2014 +0100

    baseparse: allow skipping more data than we currently have
    
    This can be useful for skipping large unwanted data, such as
    large album art, when we know the size of it from a metadata
    header.

http://lists.freedesktop.org/archives/gstreamer-commits/2014-November/083363.html

"It seems like baseparse should do something with the skip variable in a 
few more places - like clearing it on flush_stop, and maybe re-synching 
it if there's a discont or new segment?
Also, does it work properly in pull mode? Seems like it should be 
possible to skip even more efficiently in that case."

It is cleared on flush-stop.

I'm not sure what would be the best to do if there's a discont or new segment. An upstream query for byte position to determine the byte size of the discont, and keep track of where we are in bytes as we go ?

You're right that it's buggy in pull mode. While it does work (with gst-launch anyway), it is actually slower than without the patch :/ I'll have a look at why that is.

Created attachment 290643 [details] [review]
jump over large skips in pull mode

This fixes the slowdown in pull mode.

That leaves the question of what to do on a segment/discont that's not accompanied by a flush, and I'm not sure what to do, if anything.

(In reply to comment #10)
> That leaves the question of what to do on a segment/discont that's not
> accompanied by a flush, and I'm not sure what to do, if anything.

Normally just proxy them, making sure to update internal state. You'll get these if in segment seek. The segment part is not an issue, as there is no jump in time. The discount is something for the implementation to handle (e.g. updating codec_data, caps if needed).

I pushed that one, as it's a clear improvement. I don't see a clear preference to clear skip on discont or segments. I think it's valid to get a new segment that just continues at current position.


commit dd40d99710209435b34dc33839bbcced6d8b70a7
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Thu Nov 13 14:53:59 2014 +0000

    baseparse: jump over large skips in pull mode
    
    This bypasses the dumping of buffers we still have to do in push mode.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=730053

Created attachment 299299 [details] [review]
reset skip on segments and discontinuities

Large scale skip is an optimization, and thus it is safer to
stop skipping than to continue. Clear skip on segments and
discontinuities, as these are points where it is possible that
the original idea of "bytes to skip" changes.


I think that's the safest thing to do, and hopefully this closes the bug.
I'll push that in the next few days if there's no comment.

Created attachment 299310 [details] [review]
reset skip on segments and discontinuities

Buffer can be NULL here.

commit 780f71c4d504caf369504ef38ace863d2a797195
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Fri Mar 13 11:08:25 2015 +0000

    baseparse: reset skip on segments and discontinuities
    
    Large scale skip is an optimization, and thus it is safer to
    stop skipping than to continue. Clear skip on segments and
    discontinuities, as these are points where it is possible that
    the original idea of "bytes to skip" changes.