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 697103 - osxaudio: port to 1.0
osxaudio: port to 1.0
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Mac OS
: High enhancement
: 1.0.7
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 697913 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-04-02 13:50 UTC by Takashi Nakajima
Modified: 2013-04-18 07:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch file for sys/osxaudio from git master for v.1.0.x APIs. (58.54 KB, patch)
2013-04-02 13:50 UTC, Takashi Nakajima
none Details | Review
patch index by git format-patch (81 bytes, patch)
2013-04-02 14:28 UTC, Takashi Nakajima
none Details | Review
patch1: for osxaudiosink and src (59.00 KB, patch)
2013-04-02 14:32 UTC, Takashi Nakajima
committed Details | Review
Patch2: fix osxaudiosrc layout (1.16 KB, patch)
2013-04-02 14:32 UTC, Takashi Nakajima
committed Details | Review
build fixes (2.89 KB, patch)
2013-04-05 08:06 UTC, Philippe Normand
committed Details | Review
additional patch to fix up the first patch according to Sebastian's comments (5.32 KB, patch)
2013-04-09 16:34 UTC, Takashi Nakajima
committed Details | Review
replace base class with GstAudioSink to use ::prepare() (4.88 KB, patch)
2013-04-10 12:15 UTC, Takashi Nakajima
none Details | Review
call set_channel_positions() in osxaudioringbuffer acquire(). (2.72 KB, patch)
2013-04-10 14:31 UTC, Takashi Nakajima
needs-work Details | Review
update to latest glib mutex api (2.66 KB, patch)
2013-04-12 22:46 UTC, Todd Agulnick
committed Details | Review
osxaudio: use GST_AUDIO_INFO_* accessors (3.65 KB, patch)
2013-04-13 00:09 UTC, Todd Agulnick
committed Details | Review
call set_channel positions() in osxaudioringbuffer acquire() (revised) (11.15 KB, patch)
2013-04-14 05:06 UTC, Takashi Nakajima
needs-work Details | Review
call set_channel positions() in osxaudioringbuffer acquire() (revised again) (12.30 KB, patch)
2013-04-15 15:38 UTC, Takashi Nakajima
committed Details | Review
use GST_IS_OSX_AUDIO_SINK to distinguish sink from src. (1.63 KB, patch)
2013-04-16 14:44 UTC, Takashi Nakajima
committed Details | Review

Description Takashi Nakajima 2013-04-02 13:50:03 UTC
Created attachment 240381 [details] [review]
patch file for sys/osxaudio from git master for v.1.0.x APIs.

Hello,
It seemed that sys/osxaudio plugins are not ported to use ver.1.0.x APIs,
so I tried to write patches from current git master repositry.
Could someone test them?
Comment 1 Tim-Philipp Müller 2013-04-02 14:09:26 UTC
Great, thanks! Could you attach a patch in "git format-patch" format by any chance?
Comment 2 Takashi Nakajima 2013-04-02 14:28:55 UTC
Created attachment 240389 [details] [review]
patch index by git format-patch
Comment 3 Takashi Nakajima 2013-04-02 14:32:00 UTC
Created attachment 240390 [details] [review]
patch1: for osxaudiosink and src
Comment 4 Takashi Nakajima 2013-04-02 14:32:51 UTC
Created attachment 240391 [details] [review]
Patch2: fix osxaudiosrc layout
Comment 5 Takashi Nakajima 2013-04-02 14:37:51 UTC
These 3 files are made from "git format-patch".
I am newbie to send patch, so please inform me if these are't good for you.
(I think these 2 commits should be merge to one commit...?)
Comment 6 Tim-Philipp Müller 2013-04-02 15:00:35 UTC
Looks good, thanks. However, if you could run:

  git config --global user.name "Takashi Nakajima"

that would be great - then your name will be set properly in the authors field as well (for next time).
Comment 7 Tim-Philipp Müller 2013-04-02 15:00:58 UTC
Comment on attachment 240389 [details] [review]
patch index by git format-patch

This one's not needed.
Comment 8 Takashi Nakajima 2013-04-02 15:13:54 UTC
Thank you for your comment. I fixed my user.name.
Comment 9 Philippe Normand 2013-04-05 08:06:13 UTC
Created attachment 240701 [details] [review]
build fixes

The sink works, pretty nice!
Comment 10 Sebastian Dröge (slomo) 2013-04-06 19:16:56 UTC
Review of attachment 240390 [details] [review]:

Looks good in general, just some minor problems:

::: sys/osxaudio/gstosxaudiosink.c
@@ +160,3 @@
+G_DEFINE_TYPE_WITH_CODE (GstOsxAudioSink, gst_osx_audio_sink,
+    GST_TYPE_AUDIO_BASE_SINK,
+    gst_osx_audio_sink_do_init (GST_TYPE_AUDIO_BASE_SINK));

The GType is available here as g_define_type_id. You pass the wrong type to the function here

@@ +556,3 @@
       switch (layout->mChannelDescriptions[i].mChannelLabel) {
         case kAudioChannelLabel_Left:
+          channel_mask |= GST_AUDIO_CHANNEL_POSITION_MASK (FRONT_LEFT);

This is not correct, you need to tell the ringbuffer the OSX specific channel order with gst_audio_ring_buffer_set_channel_positions() additional to that mask
See existing usage of that function

::: sys/osxaudio/gstosxaudiosrc.c
@@ +126,3 @@
+G_DEFINE_TYPE_WITH_CODE (GstOsxAudioSrc, gst_osx_audio_src,
+    GST_TYPE_AUDIO_BASE_SRC,
+    gst_osx_audio_src_do_init (GST_TYPE_AUDIO_BASE_SRC));

Same here
Comment 11 Tim-Philipp Müller 2013-04-09 08:36:08 UTC
Takashi Nakajima: do you think you'll have time to fix up the first patch according to Sebastian's comments, or would you prefer it if someone else did that?
Comment 12 Takashi Nakajima 2013-04-09 16:34:58 UTC
Created attachment 241078 [details] [review]
additional patch to fix up the first patch according to Sebastian's comments

I tried to fix up the first patch, but it seems that there is wrong position where
gst_audio_ring_buffer_set_channel_positions() placed.
("assertion 'buf->acquired' failed" message)
Could someone help me?
Comment 13 Sebastian Dröge (slomo) 2013-04-09 16:47:22 UTC
You would do that for example in GstAudioSink::prepare(). It must be done when the ringbuffer is acquired.
Comment 14 Takashi Nakajima 2013-04-10 12:15:20 UTC
Created attachment 241140 [details] [review]
replace base class with GstAudioSink to use ::prepare()

Thanks a lot, Sebastian. I replaced base class from GstAudioBaseSink to GstAudioSink
to use GstAudioSink::prepare().
Comment 15 Sebastian Dröge (slomo) 2013-04-10 12:29:01 UTC
That's not going to work as osxaudiosink uses a custom ringbuffer. You should stay at GstAudioBaseSink.

GstAudioSink::prepare() is called from GstAudioRingBuffer::acquire(),so you could just call that in the osxaudioringbuffer acquire function.
Comment 16 Takashi Nakajima 2013-04-10 14:31:16 UTC
Created attachment 241165 [details] [review]
call set_channel_positions() in osxaudioringbuffer acquire().

I came back to GstAudioBaseSink, and call get_audio_ring_buffer_set_channel_positions()
from gst_osx_audio_ring_buffer_acquire().
How about this?
Comment 17 Sebastian Dröge (slomo) 2013-04-10 14:46:13 UTC
Review of attachment 241165 [details] [review]:

::: sys/osxaudio/gstosxaudioringbuffer.c
@@ +237,3 @@
+  gst_structure_get (s, "channel-mask", GST_TYPE_BITMASK, &channel_mask, NULL);
+  gst_audio_channel_positions_from_mask (channels, channel_mask, positions);
+  gst_audio_ring_buffer_set_channel_positions (buf, positions);

This is still not correct, you should pass an array of the order of the channel positions as they're used in OSX. What you pass here is the order used in GStreamer, which is most probably not the same.

Otherwise, this is now called in the right place and all that :)
Comment 18 Takashi Nakajima 2013-04-12 16:11:54 UTC
It seems that I should store OSX channel positions somewhere in order to use
later in GstOsxAudioRingBuffer::acquire().
But I'm not certain where the values should be stored. Should I store in
GstOsxAudioSink object? Or should I store in GstStructure as GValue?
(the way in old gst_audio_set_channel_positions())
Comment 19 Tim-Philipp Müller 2013-04-12 21:39:19 UTC
*** Bug 697913 has been marked as a duplicate of this bug. ***
Comment 20 Todd Agulnick 2013-04-12 22:46:10 UTC
Created attachment 241420 [details] [review]
update to latest glib mutex api

Hi, joining in from Bug 697913.

I'm building against glib 2.36.0, where g_mutex_new has been removed. The attached patch updates to the current glib threading api.
Comment 21 Todd Agulnick 2013-04-13 00:09:56 UTC
Created attachment 241423 [details] [review]
osxaudio: use GST_AUDIO_INFO_* accessors

Nakajima-san,

I hadn't realized that you were porting osxaudio to 1.0 while I was working on the same thing. Most of our changes are essentially the same, but I've attached a patch where there is one difference. It should be functionally equivalent but should make the code a bit more resilient against future data structure changes.
Comment 22 Takashi Nakajima 2013-04-13 03:43:46 UTC
Thanks, Todd-san,
I am glad to hear that you are working on the module. I think that your code
is better. I had forgotten to use those macro.
And, did you write code to set channel positions into ring buffer?
Comment 23 Todd Agulnick 2013-04-13 07:41:57 UTC
Nakajima-san,

Arigatou gozaimashita.

I got as far as you did regarding channel layout (i.e., converting from the positions array as used in 0.10 to the channel-mask as used in 1.0). I just spent some time looking at it, I think I finally understand it.

Sebastian was pointing you in the right direction. The code from osxaudiosink.c that calls gst_core_audio_audio_device_get_channel_layout() and then iterates over the results to build a position map needs to move out of osxaudiosink.c and into the osxaudioringbuffer.c's acquire function. Once you have constructed that position map, you can call the ring buffer's set_position function.

If that makes sense to you and you are comfortable implementing it, please give it a try. Otherwise I can put together a patch for you to review.

Yoroshiku!
Comment 24 Takashi Nakajima 2013-04-14 05:06:18 UTC
Created attachment 241480 [details] [review]
call set_channel positions() in osxaudioringbuffer acquire() (revised)

Todd-san,

Thank you for your suggestion. I tried to move the code into osxaudioringbuffer.c.
(and includes your patch)
Is this suits your suggestion?

Kochirakoso Yoroshiku! :)
Comment 25 Sebastian Dröge (slomo) 2013-04-14 07:18:31 UTC
Review of attachment 241480 [details] [review]:

Yes, thanks :) Just one other small issue with this:

::: sys/osxaudio/gstosxaudiosink.c
@@ -621,3 @@
-    gst_structure_remove_fields (out_s, "channels", "channel-mask", NULL);
-    gst_structure_set (out_s, "channels", G_TYPE_INT, max_channels,
-        "channel-mask", GST_TYPE_BITMASK, channel_mask, NULL);

get_allowed_caps() and get_caps() should still return the channel-mask in the caps to make sure upstream does not provide channel layouts that are unsupported
Comment 26 Takashi Nakajima 2013-04-14 14:15:59 UTC
OK, now my thought is...

  1) In get_allowed_caps(), get channel layouts from OSX and store them into the ring buffer.
      And return the channel-mask in the caps. The caps is stored as cached_caps.
  2) In get_caps(), use cached_caps in the sink (unchanged).
  3) In ring_buffer_acquire(), use channel layouts stored at 1) in the ring buffer.

Can I use GstAudioRingBuffer::spec.info.position[] to store layouts at 1)?
Or needs new variable for it?
Comment 27 Sebastian Dröge (slomo) 2013-04-14 14:28:48 UTC
Sounds like a plan but you should use a different place for storing the layouts, probably a new array of GstAudioChannelPosition[] in osxaudioringbuffer.
Comment 28 Takashi Nakajima 2013-04-15 15:38:35 UTC
Created attachment 241577 [details] [review]
call set_channel positions() in osxaudioringbuffer acquire() (revised again)

Hello,
Because the ring buffer is not created when sink_allowed_caps() called, I stored channel positions
in the sink object.
Comment 29 Todd Agulnick 2013-04-15 17:28:43 UTC
Review of attachment 241577 [details] [review]:

Nakajima-san,

I was wondering too about the timing of caps requests and the transition from the null to ready state (which I think is when the ring buffer gets created). I'm surprised to hear that the caps queries happen first.

-Todd

P.S. Gomen nasai about my earlier advice; I'm still trying to under the caps negotiation protocol.

::: sys/osxaudio/gstosxaudioringbuffer.c
@@ +229,3 @@
       CORE_AUDIO_FORMAT_ARGS (format));
 
+  osxsink = GST_OSX_AUDIO_SINK (GST_OBJECT_PARENT (buf));

This osxringbuffer is used by both the osxaudiosrc as well as the osxaudiosink. So I think this code will fail in the src case.
Comment 30 Sebastian Dröge (slomo) 2013-04-16 09:06:11 UTC
(In reply to comment #29)

> I was wondering too about the timing of caps requests and the transition from
> the null to ready state (which I think is when the ring buffer gets created).
> I'm surprised to hear that the caps queries happen first.

The CAPS query and ACCEPT_CAPS query can happen at any time, but before the element has transitioned to the READY state it's normal to just use the template caps here. In READY/PAUSED/PLAYING you would check what the device actually supports
Comment 31 Sebastian Dröge (slomo) 2013-04-16 10:54:58 UTC
Review of attachment 241577 [details] [review]:

Looks good in general, just:

::: sys/osxaudio/gstosxaudioringbuffer.c
@@ +229,3 @@
       CORE_AUDIO_FORMAT_ARGS (format));
 
+  osxsink = GST_OSX_AUDIO_SINK (GST_OBJECT_PARENT (buf));

Yes, that part will fail for the source. GST_IS_OSX_AUDIO_SINK() can help here... but similar code should probably also be added for the source here.
Comment 32 Takashi Nakajima 2013-04-16 14:44:34 UTC
Created attachment 241647 [details] [review]
use GST_IS_OSX_AUDIO_SINK to distinguish sink from src.

Hello, Todd, and Sebastian,

Thank you for your review.
I implemented GST_IS_OSX_AUDIO_SINK and set_position only if the object is sink.

But, what do you mean "similar code", Sebastian?
Should I inspect source device channel layout and store as channel-mask in get_caps()?

P.S.
Todd-san, I really thank you because that you consider the problem with me.
Doumo Arigatou.
Comment 33 Sebastian Dröge (slomo) 2013-04-16 15:08:33 UTC
(In reply to comment #32)

> But, what do you mean "similar code", Sebastian?
> Should I inspect source device channel layout and store as channel-mask in
> get_caps()?

Yes
Comment 34 Sebastian Dröge (slomo) 2013-04-16 15:08:51 UTC
Review of attachment 241647 [details] [review]:

::: sys/osxaudio/gstosxaudioringbuffer.c
@@ +231,2 @@
   osxsink = GST_OSX_AUDIO_SINK (GST_OBJECT_PARENT (buf));
+  if (GST_IS_OSX_AUDIO_SINK (osxsink)) {

First check, then do the GST_OSX_AUDIO_SINK() cast
Comment 35 Sebastian Dröge (slomo) 2013-04-17 07:52:57 UTC
commit 2b1f967101ebbf35629b5b6abd19e30e93f04a6b
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Wed Apr 17 09:50:43 2013 +0200

    osxaudioringbuffer: First check the type, then cast

commit ce5246ed71689f6ec027b921f2c68b8d1813a6dd
Author: Takashi Nakajima <ted.nakajima@gmail.com>
Date:   Tue Apr 16 22:46:00 2013 +0900

    osxaudio: use GST_IS_OSX_AUDIO_SINK in ring buffer.

commit efda79b0845e99a04ea6c07fa6e3c81e4c7bf6b7
Author: Takashi Nakajima <ted.nakajima@gmail.com>
Date:   Wed Apr 10 21:06:16 2013 +0900

    osxaudio: call set_channel_positions() in osxaudioringbuffer acquire()

commit c2c85a094a7cddc3a2452922c9b1857ae2ba1d71
Author: Todd Agulnick <todd@agulnick.com>
Date:   Fri Apr 12 12:18:04 2013 -0700

    osxaudio: use GST_AUDIO_INFO_* accessors
    
    Changes include the following:
    
     * Update classname references
     * Replace GST_BOILERPLATE_FULL with G_DEFINE_TYPE
     * Use new GstAudioInfo struct and methods
     * Use new buffer memory allocation scheme
    
    Conflicts:
    	sys/osxaudio/gstosxaudioringbuffer.c

commit ed94ef79f9a2de254c6b1e97bd30fe7838b3a6e6
Author: Todd Agulnick <todd@agulnick.com>
Date:   Fri Apr 12 11:51:46 2013 -0700

    osxaudio: adjust for changes to glib mutex api.

commit 09e980d2c93429dc71f0df1496da75b067be14ff
Author: Takashi Nakajima <ted.nakajima@gmail.com>
Date:   Wed Apr 10 01:21:49 2013 +0900

    osxaudio: try to fix up according to Sebastian's comments

commit ab64837bf21c3db58449a4e6043499a1ae85746d
Author: Philippe Normand <philn@igalia.com>
Date:   Fri Apr 5 10:02:38 2013 +0200

    osxaudio: build fixes
    
    Enable the osxaudio plugin build in configure.ac and fix some
    include directive order issues.

commit d5d53ec6118dde3e5989d8bd3b1e9ffedc7e34f6
Author: ted-n <ted.nakajima@gmail.com>
Date:   Tue Apr 2 22:28:09 2013 +0900

    osxaudio: fix layout for osxaudiosrc

commit b217b6fdfbefeb63ee7cd6d7bb348984160a37c1
Author: ted-n <ted.nakajima@gmail.com>
Date:   Sat Mar 30 22:49:34 2013 +0900

    osxaudio: port to v.1.0