Created attachment 304157 [details] [review]
basetransform: Split input buffer processing from output generation

Allow for sub-classes which want to collate incoming buffers or
split them into multiple output buffers by separating the input
buffer submission from output buffer generation and allowing
for looping of one of the phases depending on pull or push mode
operation.

Review of attachment 304157 [details] [review]:

I'm worried this is a partial solution to the need for a proper AsyncBaseTransform.

::: libs/gst/base/gstbasetransform.h
@@ +93,3 @@
+  /* Default submit_input_buffer places the buffer here,
+   * for consumption by the generate_output method: */
+  GstBuffer      *queued_buf;

What if the element would like to handle it's own queue ?

@@ +279,3 @@
   GstFlowReturn (*transform_ip) (GstBaseTransform *trans, GstBuffer *buf);
 
+  GstFlowReturn (*submit_input_buffer) (GstBaseTransform *trans, gboolean is_discont, GstBuffer *input);

Other similar API uses the term finish, though I'm not sure how to make that fit. Why do you need the discount boolean ? Isn't the discont flag on the buffer you submit enough ?

@@ +280,3 @@
 
+  GstFlowReturn (*submit_input_buffer) (GstBaseTransform *trans, gboolean is_discont, GstBuffer *input);
+  GstFlowReturn (*generate_output) (GstBaseTransform *trans, GstBuffer **outbuf);

generate or create (create seems more popular so far).

(In reply to Nicolas Dufresne (stormer) from comment #2)
> Review of attachment 304157 [details] [review] [review]:
> 
> I'm worried this is a partial solution to the need for a proper
> AsyncBaseTransform.

If you mean a base transform that implements a separate srcpad task for outputting buffers, this should allow that - override the generate_output() vfunc to just collect input buffers to somewhere for processing, and implement whatever it wants for the srcpad task. The first person to actually implement one might find some wrinkles to iron out, but it's feasible.
 
> ::: libs/gst/base/gstbasetransform.h
> @@ +93,3 @@
> +  /* Default submit_input_buffer places the buffer here,
> +   * for consumption by the generate_output method: */
> +  GstBuffer      *queued_buf;
> 
> What if the element would like to handle it's own queue ?

The pattern is for a sub-class to implement submit_input_buffer() as:

  GST_BASE_TRANSFORM_CLASS (parent)->submit_input_buffer(...) || fail;
  /* Parent submit succeeded,
     take any buffer from btrans->queued_buf and store it */

So, the default implementation does QoS as needed and places the buffer into the queued_buf ptr. The default generate_output() function looks for a buffer there and processes it, but the sub-class is free to do something else instead - I'm doing something like the above pseudo-code in the stereoscopic branch.

Sub-classes are free to implement their own queueing, and can still use the default generate_output() function by placing buffers into the queued_buf ptr.

I did it this way so that everything is backwards-compatible by default - 1 input buffer yields 1 output buffer.

> @@ +279,3 @@
>    GstFlowReturn (*transform_ip) (GstBaseTransform *trans, GstBuffer *buf);
>  
> +  GstFlowReturn (*submit_input_buffer) (GstBaseTransform *trans, gboolean
> is_discont, GstBuffer *input);
> 
> Other similar API uses the term finish, though I'm not sure how to make that
> fit.

We use finish() in places where the sub-class wants a parent class to take a buffer it's created and do something with it, but this is a receive function for input buffers. 

> Why do you need the discount boolean ? Isn't the discont flag on the
> buffer you submit enough ?

As well as the input buffer DISCONT flag, the is_discont param also reflects whether or not basetransform dropped a buffer due to QoS since the last one was input. Passing it as a separate param prevents having to make the input buffer writable and set its DISCONT flag in that case.
 
> @@ +280,3 @@
>  
> +  GstFlowReturn (*submit_input_buffer) (GstBaseTransform *trans, gboolean
> is_discont, GstBuffer *input);
> +  GstFlowReturn (*generate_output) (GstBaseTransform *trans, GstBuffer
> **outbuf);
> 
> generate or create (create seems more popular so far).

Either one is a bit weird when there's already a prepare_output_buffer vfunc, but I'll go with consensus :)

Review of attachment 304157 [details] [review]:

The submit_input_buffer() (you could maybe call the queue_input_buffer()?) and generate_output_buffer() API also seems a bit suboptimal to me.

It seems more optimal to the default submit_input_buffer() queue one buffer, and then call generate_output_buffer() until that is done, and call some kind of finish_buffer() with the result(s). Instead of having the call to generate_output_buffer() directly in the chain/getrange functions, and magically forward the results.

That way you could have subclasses that override submit_input_buffer() to take control over the complete data flow (e.g. queue things up, have some other thread generate output and call finish()), or subclasses that have the default submit_input_buffer() and override just generate_output_buffer() to produce 0, 1 or many output buffers from one single input buffer. It seems like a more generic data flow model that can be customized more.

::: libs/gst/base/gstbasetransform.h
@@ +93,3 @@
+  /* Default submit_input_buffer places the buffer here,
+   * for consumption by the generate_output method: */
+  GstBuffer      *queued_buf;

Or what about buffer lists :)

So basically this as the default implementation, with finish_buffer() as a public basetransform function.

submit_input_buffer(inbuf):
  while (outbuf = generate_output_buffer(inbuf)):
    ret = finish_buffer(outbuf);

(In reply to Sebastian Dröge (slomo) from comment #4)
> Review of attachment 304157 [details] [review] [review]:
> 
> The submit_input_buffer() (you could maybe call the queue_input_buffer()?)
> and generate_output_buffer() API also seems a bit suboptimal to me.

My primary intent is to ensure that all existing elements see exactly the same sequence of operations if they don't explicitly override either the submit_input_buffer() or generate_output() vfuncs. Since we have a few different variants of GstBaseTransform I want to be very careful to retain backwards-compatibility.

That said, it might be possible to achieve what you suggested too.

> It seems more optimal to the default submit_input_buffer() queue one buffer,
> and then call generate_output_buffer() until that is done, and call some
> kind of finish_buffer() with the result(s). Instead of having the call to
> generate_output_buffer() directly in the chain/getrange functions, and
> magically forward the results.
> 
> That way you could have subclasses that override submit_input_buffer() to
> take control over the complete data flow (e.g. queue things up, have some
> other thread generate output and call finish()), or subclasses that have the
> default submit_input_buffer() and override just generate_output_buffer() to
> produce 0, 1 or many output buffers from one single input buffer. It seems
> like a more generic data flow model that can be customized more.

All those are possible with what I've generated, but moving the buffer pushing into a _finish() function could work. I'll try it out.

> 
> ::: libs/gst/base/gstbasetransform.h
> @@ +93,3 @@
> +  /* Default submit_input_buffer places the buffer here,
> +   * for consumption by the generate_output method: */
> +  GstBuffer      *queued_buf;
> 
> Or what about buffer lists :)

Are a problem for the person that implements buffer list support in GstBaseTransform :-P

(In reply to Sebastian Dröge (slomo) from comment #5)
> So basically this as the default implementation, with finish_buffer() as a
> public basetransform function.
> 
> submit_input_buffer(inbuf):
>   while (outbuf = generate_output_buffer(inbuf)):
>     ret = finish_buffer(outbuf);

The problem with this structure is that it doesn't map onto the existing pull-mode implementation - the output buffer function has to return the outbuf to the caller so it can be pushed or returned from the pull request appropriately.

Really the only thing I can think of to change about the patch I've proposed is the caching of the buffer in the GstBuffer *queued_buf member on GstBaseTransform. 

It could be inferred from the return result of submit_input_buffer() whether or not the passed buffer has been consumed, but I don't think that's significantly better than making it explicit and uniform.

As I said in comment #7 implementing a finish_buffer() method doesn't work for pull mode, unless I add a queue and more complexity to GstBaseTransform.