Great, thanks! Could you attach a patch in "git format-patch" format by any chance?

Created attachment 240389 [details] [review]
patch index by git format-patch

Created attachment 240390 [details] [review]
patch1: for osxaudiosink and src

Created attachment 240391 [details] [review]
Patch2: fix osxaudiosrc layout

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...?)

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 on attachment 240389 [details] [review]
patch index by git format-patch

This one's not needed.

Thank you for your comment. I fixed my user.name.

Created attachment 240701 [details] [review]
build fixes

The sink works, pretty nice!

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

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?

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?

You would do that for example in GstAudioSink::prepare(). It must be done when the ringbuffer is acquired.

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().

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.

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?

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 :)

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())

*** Bug 697913 has been marked as a duplicate of this bug. ***

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.

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.

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?

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!

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! :)

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

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?

Sounds like a plan but you should use a different place for storing the layouts, probably a new array of GstAudioChannelPosition[] in osxaudioringbuffer.

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.

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.

(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

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.

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.

(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

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

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