After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 793492 - GstAudioConverter: improve bindability
GstAudioConverter: improve bindability
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other All
: Normal enhancement
: 1.13.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-02-15 19:06 UTC by Mathieu Duponchelle
Modified: 2018-02-15 20:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gst_audio_info_set_format: annotate positions parameter (922 bytes, patch)
2018-02-15 19:06 UTC, Mathieu Duponchelle
accepted-commit_now Details | Review
AudioConverter: register as boxed type (2.42 KB, patch)
2018-02-15 19:06 UTC, Mathieu Duponchelle
committed Details | Review
gst_audio_converter_new: update annotations (1.10 KB, patch)
2018-02-15 19:06 UTC, Mathieu Duponchelle
committed Details | Review
audio-converter: add a convenience conversion method (3.52 KB, patch)
2018-02-15 19:06 UTC, Mathieu Duponchelle
committed Details | Review
audio-info: annotate gst_audio_info_set_format (913 bytes, patch)
2018-02-15 19:21 UTC, Mathieu Duponchelle
committed Details | Review

Description Mathieu Duponchelle 2018-02-15 19:06:14 UTC
These few patches make it possible to instantiate and use GstAudioConverter
from python
Comment 1 Mathieu Duponchelle 2018-02-15 19:06:18 UTC
Created attachment 368380 [details] [review]
gst_audio_info_set_format: annotate positions parameter
Comment 2 Mathieu Duponchelle 2018-02-15 19:06:23 UTC
Created attachment 368381 [details] [review]
AudioConverter: register as boxed type
Comment 3 Mathieu Duponchelle 2018-02-15 19:06:27 UTC
Created attachment 368382 [details] [review]
gst_audio_converter_new: update annotations
Comment 4 Mathieu Duponchelle 2018-02-15 19:06:32 UTC
Created attachment 368383 [details] [review]
audio-converter: add a convenience conversion method

This is useful from python bindings
Comment 5 Thibault Saunier 2018-02-15 19:11:39 UTC
Review of attachment 368380 [details] [review]:

`audioinfo:` as commit prefix? otherwise, sure.
Comment 6 Thibault Saunier 2018-02-15 19:12:24 UTC
Review of attachment 368381 [details] [review]:

Why is it necessary? Is it really the expected behaviour to get a copy of it?
Comment 7 Thibault Saunier 2018-02-15 19:12:58 UTC
Review of attachment 368382 [details] [review]:

Looks good, I wonder why it was skipped though, any idea?
Comment 8 Thibault Saunier 2018-02-15 19:15:02 UTC
Review of attachment 368383 [details] [review]:

Why is it required in python? I guess it is generally useful indeed.
Comment 9 Mathieu Duponchelle 2018-02-15 19:16:16 UTC
(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
Comment 10 Mathieu Duponchelle 2018-02-15 19:17:51 UTC
(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
Comment 11 Mathieu Duponchelle 2018-02-15 19:18:53 UTC
(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
Comment 12 Mathieu Duponchelle 2018-02-15 19:21:05 UTC
Created attachment 368385 [details] [review]
audio-info: annotate gst_audio_info_set_format
Comment 13 Mathieu Duponchelle 2018-02-15 19:21:25 UTC
(In reply to Thibault Saunier from comment #5)
> Review of attachment 368380 [details] [review] [review]:
> 
> `audioinfo:` as commit prefix? otherwise, sure.

Done
Comment 14 Thibault Saunier 2018-02-15 19:24:38 UTC
Review of attachment 368385 [details] [review]:

Go.
Comment 15 Sebastian Dröge (slomo) 2018-02-15 19:27:19 UTC
(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.
Comment 16 Thibault Saunier 2018-02-15 19:30:30 UTC
(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.
Comment 17 Thibault Saunier 2018-02-15 19:30:42 UTC
Review of attachment 368381 [details] [review]:

OK
Comment 18 Sebastian Dröge (slomo) 2018-02-15 19:39:12 UTC
(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.
Comment 19 Mathieu Duponchelle 2018-02-15 19:53:18 UTC
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