Created attachment 368380 [details] [review]
gst_audio_info_set_format: annotate positions parameter

Created attachment 368381 [details] [review]
AudioConverter: register as boxed type

Created attachment 368382 [details] [review]
gst_audio_converter_new: update annotations

Created attachment 368383 [details] [review]
audio-converter: add a convenience conversion method

This is useful from python bindings

Review of attachment 368380 [details] [review]:

`audioinfo:` as commit prefix? otherwise, sure.

Review of attachment 368381 [details] [review]:

Why is it necessary? Is it really the expected behaviour to get a copy of it?

Review of attachment 368382 [details] [review]:

Looks good, I wonder why it was skipped though, any idea?

Review of attachment 368383 [details] [review]:

Why is it required in python? I guess it is generally useful indeed.

(In reply to Thibault Saunier from comment #6)
> Review of attachment 368381 [details] [review] [review]:
> 
> Why is it necessary? Is it really the expected behaviour to get a copy of it?

The alternative is to refcount it and ideally make it thread safe, this would be possible but Sebastian was of the opinion that there was no real use case for that

(In reply to Thibault Saunier from comment #7)
> Review of attachment 368382 [details] [review] [review]:
> 
> Looks good, I wonder why it was skipped though, any idea?

Simply because GstAudioConverter was not registered as a boxed type, causing warnings to be raised by gi

(In reply to Thibault Saunier from comment #8)
> Review of attachment 368383 [details] [review] [review]:
> 
> Why is it required in python? I guess it is generally useful indeed.

because _samples expects an output buffer allocated at the right size, making things a bit awkward from python

Created attachment 368385 [details] [review]
audio-info: annotate gst_audio_info_set_format

(In reply to Thibault Saunier from comment #5)
> Review of attachment 368380 [details] [review] [review]:
> 
> `audioinfo:` as commit prefix? otherwise, sure.

Done

Review of attachment 368385 [details] [review]:

Go.

(In reply to Mathieu Duponchelle from comment #9)
> (In reply to Thibault Saunier from comment #6)
> > Review of attachment 368381 [details] [review] [review] [review]:
> > 
> > Why is it necessary? Is it really the expected behaviour to get a copy of it?
> 
> The alternative is to refcount it and ideally make it thread safe, this
> would be possible but Sebastian was of the opinion that there was no real
> use case for that

Refcounting comes with its own set of problems and increases complexity (for users as well as bindings: consider the thread-safety traits in Rust and their interaction with refcounting for example), and GstAudioConverter should not really be shared in different places. You use it from one place, otherwise you'll run into interesting problems due to it having a sample history, etc.

(In reply to Sebastian Dröge (slomo) from comment #15)
> (In reply to Mathieu Duponchelle from comment #9)
> > (In reply to Thibault Saunier from comment #6)
> > > Review of attachment 368381 [details] [review] [review] [review] [review]:
> > > 
> > > Why is it necessary? Is it really the expected behaviour to get a copy of it?
> > 
> > The alternative is to refcount it and ideally make it thread safe, this
> > would be possible but Sebastian was of the opinion that there was no real
> > use case for that
> 
> Refcounting comes with its own set of problems and increases complexity (for
> users as well as bindings: consider the thread-safety traits in Rust and
> their interaction with refcounting for example), and GstAudioConverter
> should not really be shared in different places. You use it from one place,
> otherwise you'll run into interesting problems due to it having a sample
> history, etc.

Right, it is a bit weird to copy to me, but makes things simpler, so let's go with that.

Review of attachment 368381 [details] [review]:

OK

(In reply to Thibault Saunier from comment #16)
> (In reply to Sebastian Dröge (slomo) from comment #15)
> > (In reply to Mathieu Duponchelle from comment #9)
> > > (In reply to Thibault Saunier from comment #6)
> > > > Review of attachment 368381 [details] [review] [review] [review] [review] [review]:
> > > > 
> > > > Why is it necessary? Is it really the expected behaviour to get a copy of it?
> > > 
> > > The alternative is to refcount it and ideally make it thread safe, this
> > > would be possible but Sebastian was of the opinion that there was no real
> > > use case for that
> > 
> > Refcounting comes with its own set of problems and increases complexity (for
> > users as well as bindings: consider the thread-safety traits in Rust and
> > their interaction with refcounting for example), and GstAudioConverter
> > should not really be shared in different places. You use it from one place,
> > otherwise you'll run into interesting problems due to it having a sample
> > history, etc.
> 
> Right, it is a bit weird to copy to me, but makes things simpler, so let's
> go with that.

The code that actually calls these copy functions is most likely wrong and doing something silly :) This is all really just to make gobject-introspection happy.

Attachment 368381 [details] pushed as 9046e60 - AudioConverter: register as boxed type
Attachment 368382 [details] pushed as 6a4a82f - gst_audio_converter_new: update annotations
Attachment 368383 [details] pushed as 9cf4293 - audio-converter: add a convenience conversion method
Attachment 368385 [details] pushed as 3d50d0e - audio-info: annotate gst_audio_info_set_format