NSSound Reimplementation

[Stef Bidi]

OK, I started working on the NSSound side of of the reimplementation (already finished the Source and Sink protocols) today.  I'm pretty much implementing this in the manner I explained in my previous e-mails.  I've attached a text file with some excerpts from NSSound.h and NSSound.m.  I'd like the maintainers to take a look at the ivar setup, I saw something like this in the Apple docs and thought it was a good way to mask some of the interworking of the implementation.  Another thing to look into is the way I'm using the NSConditionLock to block the streaming thread.

PS: Please note all the FIXME tags.... I just started working on this and have not even checked to see if it compiles, I just need to know I'm going in the right direction before going further.

Thanks
Stefan

_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Stef Bidi]

It would really help if I attached the file...

@interface NSSound : NSObject <NSCoding, NSCopying>
{
  NSString *_name;
  NSData   *_data;
  NSString *_playbackDeviceIdentifier; // Currently unused
  NSArray  *_channelMapping; // Currently unused
  BOOL     _onlyReference;
  id       _delegate;
  NSTimeInterval _duration;
 
  // Private info:
  //   0 - input bundle (id<GSSoundSource>)
  //   1 - output bundle (id<GSSoundSink>)
  //   2 - lock (NSConditionLock *)
  void *_private[4];
  BOOL _shouldStop;
}



- (void)_stream
{
  NSUInteger bytesRead;
  BOOL bytesWereWritten;
  BOOL success;
  void *bytes;
 
  bytes = NSZoneMalloc(NSDefaultMallocZone(), BUFFER_SIZE);
  bytesRead = [(id<GSSoundSource>)_private[0] readBytes: bytes length: BUFFER_SIZE];
  while ((!_shouldExit) || (bytesRead > 0) || (!success))
    {
      /* FIXME */
      if ([(NSConditionLock*)_private[2] condition] == SOUND_SHOULD_PAUSE)
        {
          [(NSConditionLock*)_private[2] lockWhenCondtion: SOUND_SHOULD_PLAY];
          [(NSConditionLock*)_private[2] unlock];
        }
      success = [(id<GSSoundSink>)_private[1] writeBytes: bytes length: bytesRead];
      bytesRead = [(id<GSSoundSource>)_private[0] readBytes: bytes length: BUFFER_SIZE];
    }
 
  [self performSelectorOnMainThread: @selector(_finished:)
                         withObject: [NSNumber numberforBool: success]
                      waitUntilDone: NO];
}

- (BOOL) pause
{
  if ([(NSConditionLock*)_private[2] condition] == SOUND_SHOULD_PAUSE)
    {
      return NO;
    }
  if ([(NSConditionLock*)_private[2] tryLock] == NO)
    {
      return NO;
    }
  [(NSConditionLock*)_private[2] unlockWithCondition: SOUND_SHOULD_PAUSE];
  return YES;
}

- (BOOL) play
{
  // If the NSCondtionLock exists it's because NSSound is either playing or paused
  if ((NSConditionLock*)_private[2] != nil)
    {
      return NO;
    }
 
  _private[2] = (void *)[NSCondtionLock initWithCondition: SOUND_SHOULD_PAUSE];
  /* FIXME */
  if ([(NSConditionLock*)_private[2] tryLock] != YES)
    {
      return NO;
    }
 
  [[(NSConditionLock*)_private[2] unlockWithCondition: SHOUND_SHOULD_PLAY];
  return YES;
}

- (BOOL) resume
{
  if ([(NSConditionLock*)_private[2] condition] == SOUND_SHOULD_PLAY)
    {
      return NO;
    }
  if ([(NSConditionLock*)_private[2] tryLock] == NO)
    {
      return NO;
    }
  [(NSConditionLock*)_private[2] unlockWithCondition: SOUND_SHOULD_PLAY];
  return YES;
}

- (BOOL) stop
{
  if ((NSConditionLock*)_private[2] == nil)
    {
      return NO;
    }
  /* FIXME */
  _shouldStop = YES;
 
  RELEASE((NSConditionLock*)_private[2]);
 
  return YES;
}

- (BOOL) isPlaying
{
  if ([(NSConditionLock*)_private[2] condition] == SOUND_SHOULD_PLAY)
    {
      return YES;
    }
  return NO;
}

_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Fred Kiefer]

Stefan Bidigaray wrote:
> It would really help if I attached the file...
>

I don't understand the general concept here, so just a few detail
comments on the code itself.

Hiding the implementation details in the header is a good thing, but it
makes the code in the implementation harder to read. This could be
improved by using local variables (or even Macros). For example


- (BOOL) pause
{
  NSConditionLock *lock = (NSConditionLock*)_private[2];

  if ([lock condition] == SOUND_SHOULD_PAUSE)
    {
      return NO;
    }
  if ([lock tryLock] == NO)
    {
      return NO;
    }
  [lock unlockWithCondition: SOUND_SHOULD_PAUSE];
  return YES;
}

When releasing the conditional lock, you should rather destroy it, as it
could otherwise just be reused, although it has been freed.


Shouldn't the condition of the while loop read something like this?
  while ((!_shouldExit) && (bytesRead > 0) && success)



_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Richard Frith-Macdonald-2]

On 19 Jun 2009, at 08:05, Fred Kiefer wrote:

> Stefan Bidigaray wrote:
>> It would really help if I attached the file...
>>
>
> I don't understand the general concept here, so just a few detail
> comments on the code itself.
>
> Hiding the implementation details in the header is a good thing, but  
> it
> makes the code in the implementation harder to read. This could be
> improved by using local variables (or even Macros). For example
>
>
> - (BOOL) pause
> {
>  NSConditionLock *lock = (NSConditionLock*)_private[2];
>
>  if ([lock condition] == SOUND_SHOULD_PAUSE)
>    {
>      return NO;
>    }
>  if ([lock tryLock] == NO)
>    {
>      return NO;
>    }
>  [lock unlockWithCondition: SOUND_SHOULD_PAUSE];
>  return YES;
> }

I would go further and ask what the point of the _private array is?

By convention all the instance variables are private (that's what the  
leading underscore in the variable name means), so if all that's  
required is to inform developers that they should not use the ivars  
directly, there's no point in having the array, but maybe it would be  
worth using a @private declaration.

On the other hand, if the idea is to hide the implementation details  
so that ivar layouts won't need to change with future revisions  
(something I think we should all be doing), my preference would be to  
have a single pointer to a structure containing the required data.
eg.

struct _NSSoundInternal;

@interface NSSound : NSObject <NSCoding, NSCopying>
{
@private
   struct _NSSoundInternal *_internal;
}

Then the designated initialiser allocates memory for the structure,  
clears it, and assigns the pointer when an instance is initialised,  
and -dealloc frees the memory at the end, and all internal references  
to ivars just go indirect from that pointer.  The implementation is  
completely hidden/private and can be changed in subsequent releases  
without breaking the ABI.





_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Stef Bidi]

On Fri, Jun 19, 2009 at 2:05 AM, Fred Kiefer <fredkiefer@...> wrote:

I don't understand the general concept here, so just a few detail
comments on the code itself.

I'll try to explain this the best I can, it might be a little long, though...

As I explained on my previous e-mails, I'm streaming the data to the sink.  In order to do so without blocking the main thread I'm using a separate thread (spawned using -detachNewThreadSelector:toTarget:withObject:)... this will happen in -play and the selector is -_stream.  The stream uses 2 objects, one conforms to <GSSoundSource> and the other to <GSSoundSink>, the former outputs the data in a standard format so that the latter can send it to the audio device.  To pause and resume the stream I use an NSConditionLock, which is what _private[2] is, if the condition is SOUND_SHOULD_PAUSE the while loop waits until it's SOUND_SHOULD_PLAY.  The hardest part, at least for me, is finding out if the thread is already running in -play and -stop and taking the appropiate action.  The -pause and -resume methods are pretty straight forward.

Hiding the implementation details in the header is a good thing, but it
makes the code in the implementation harder to read. This could be
improved by using local variables (or even Macros). For example

Sounds good!

When releasing the conditional lock, you should rather destroy it, as it
could otherwise just be reused, although it has been freed.

Wouldn't this cause a bit of a problem in -_stream since I keep checking the NSConditionLock's condition?  My plan was to initialize it in -play and free/destroy it in -stop and use it as the way to find out if the audio data is being streamed.

Shouldn't the condition of the while loop read something like this?
 while ((!_shouldExit) && (bytesRead > 0) && success)

Yes!  Oops...

I've attached a copy of GSSoundSource.h and GSSoundSink.h, these two header files define the protocols and give you an idea of what exactly I'm going for.  I've also gone ahead and added documention as well.  I'll also go ahead and attach the first two classes that implement these protocols.  As before, I haven't had time to test them, so I'm sure there are typos everywhere.  I do plan on testing them later today, make sure they actually do what they're supposed to before finishing the NSSound implentation.

Thanks
Stefan
 

[GSSoundSink.h]

---

/*
   GSSoundSink.h

   Sink audio data.

   Copyright (C) 2009 Free Software Foundation, Inc.

   Written by:  Stefan Bidigaray <stefanbidi@...>
   Date: Jun 2009
   
   This file is part of the GNUstep GUI Library.

   This library is free software; you can redistribute it and/or
   modify it under the terms of the GNU Lesser General Public
   License as published by the Free Software Foundation; either
   version 2 of the License, or (at your option) any later version.

   This library is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
   Lesser General Public License for more details.

   You should have received a copy of the GNU Lesser General Public
   License along with this library; see the file COPYING.LIB.
   If not, see <http://www.gnu.org/licenses/> or write to the
   Free Software Foundation, 51 Franklin Street, Fifth Floor,
   Boston, MA 02110-1301, USA.
*/

#ifndef _GNUstep_H_GSSoundSink
#define _GNUstep_H_GSSoundSink

@protocol GSSoundSink <NSObject>

/** <init/>
 *  Initializes the receiver for output using the defined parameters.
 *  <p><b>WARNING:</b> This method does not open the device, see -open.</p>
 */
- (id)initWithEncoding: (int)encoding
              channels: (NSUInteger)channelCount
            sampleRate: (NSUInteger)sampleRate
             byteOrder: (NSByteOrder)byteOrder;
/** Opens the device for output, called by [NSSound-play].
 */
- (BOOL)open;
/** Closes the device, called by [NSSound-stop].
 */
- (void)close;
/** Writes/Plays the data in bytes to the device.  Data <i>must</i> be in
 *  the same format as specified in
 *  -initWithEncoding:channels:sampleRate:byteOrder:.
 */
- (BOOL)writeBytes: (void *)bytes length: (NSUInteger)length;

/** Called by [NSSound-setVolume:], and corresponds to it.  Parameter volume
 *  is between the values 0.0 and 1.0.
 */
- (void)setVolume: (float)volume;
/** Called by [NSSound-volume].
 */
- (float)volume;
/** Called by [NSSound-setPlaybackDeviceIdentifier:].
 */
- (void)setPlaybackDeviceIdentifier: (NSString *)playbackDeviceIdentifier;
/** Called by [NSSound-playbackDeviceIdentifier].
 */
- (NSString *)playbackDeviceIdentifier;
/** Called by [NSSound-setChannelMapping:].
 */
- (void)setChannelMapping: (NSArray *)channelMapping;
/** Called by [NSSound-channelMapping].
 */
- (NSArray *)channelMapping;

@end

#endif // _GNUstep_H_GSSoundSink

---

[GSSoundSource.h]

---

/*
   GSSoundSource.h

   Load and read sound data.

   Copyright (C) 2009 Free Software Foundation, Inc.

   Written by:  Stefan Bidigaray <stefanbidi@...>
   Date: Jun 2009
   
   This file is part of the GNUstep GUI Library.

   This library is free software; you can redistribute it and/or
   modify it under the terms of the GNU Lesser General Public
   License as published by the Free Software Foundation; either
   version 2 of the License, or (at your option) any later version.

   This library is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
   Lesser General Public License for more details.

   You should have received a copy of the GNU Lesser General Public
   License along with this library; see the file COPYING.LIB.
   If not, see <http://www.gnu.org/licenses/> or write to the
   Free Software Foundation, 51 Franklin Street, Fifth Floor,
   Boston, MA 02110-1301, USA.
*/

#ifndef _GNUstep_H_GSSoundSource
#define _GNUstep_H_GSSoundSource

@class NSArray;

@protocol GSSoundSource <NSObject>

/** Returns an array of the file types supported by the class.
 */
+ (NSArray *)soundUnfilteredFileTypes;
/** Returns an array of UTIs identifying the file types the class understands.
 */
+ (NSArray *)soundUnfilteredTypes;
/** Returns YES if the class can understand data and NO otherwise.
 */
+ (BOOL)canInitWithData: (NSData *)data;

/** <init />
 *  Initilizes the reciever for output.
 */
- (id)initWithData: (NSData *)data;
/** Reads data provided in -initWithData:.  Parameter bytes must be big enough
 *  to hold length bytes.
 */
- (NSUInteger)readBytes: (void *)bytes length: (NSUInteger)length;

/** Returns the duration, in seconds.  Equivalent to [NSSound-duration].
 */
- (NSTimeInterval)duration;
/** Called by [NSSound-setCurrentTime:].
 */
- (void)setCurrentTime: (NSTimeInterval)currentTime;
/** Called by [NSSound-currentTime].
 */
- (NSTimeInterval)currentTime;

/** Returns encoding of the audio data.
 */
- (int)encoding;
/** Returns the number of channels.
 */
- (NSUInteger)channelCount;
/** Returns the receiver's sample rate (ie 44100, 8000, etc).
 */
- (NSUInteger)sampleRate;
/** Returns the byte order of the audio data.
 */
- (NSByteOrder)byteOrder;

@end

#endif // _GNUstep_H_GSSoundSource

---

[AudioOutputSink.m]

---

/*
   AudioOutputSink.m

   Sink audio data to libao.

   Copyright (C) 2009 Free Software Foundation, Inc.

   Written by:  Stefan Bidigaray <stefanbidi@...>
   Date: Jun 2009
   
   This file is part of the GNUstep GUI Library.

   This library is free software; you can redistribute it and/or
   modify it under the terms of the GNU Lesser General Public
   License as published by the Free Software Foundation; either
   version 2 of the License, or (at your option) any later version.

   This library is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
   Lesser General Public License for more details.

   You should have received a copy of the GNU Lesser General Public
   License along with this library; see the file COPYING.LIB.
   If not, see <http://www.gnu.org/licenses/> or write to the
   Free Software Foundation, 51 Franklin Street, Fifth Floor,
   Boston, MA 02110-1301, USA.
*/

#include <Foundation/Foundation.h>
#include <AppKit/NSSound.h>
#include <GNUstepGUI/GSSoundInput.h>
#include <ao/ao.h>

@interface AudioOutputSink : NSObject <GSSoundSink>
{
  ao_device *_dev;
  int _driver;
  ao_sample_format _format;
}
@end

@implementation AudioOutputSink

- (void)dealloc
{
  ao_shutdown();
 
  [super dealloc];
}

- (id)initWithEncoding: (int)encoding
              channels: (NSUInteger)channelCount
            sampleRate: (NSUInteger)sampleRate
             byteOrder: (NSByteOrder)byteOrder
{
  self = [super init];
 
  ao_initialize();
  _driver = ao_default_driver_id();
       
  _format.channels = (int)channelCount;
  _format.rate = (int)sampleRate;
 
  switch (encoding)
    {
      case GSSoundFormatPCMS8:
        _format.bits = 8;
        break;
     
      case GSSoundFormatPCM16:
        _format.bits = 16;
        break;
     
      case GSSoundFormatPCM24:
        _format.bits = 24;
        break;
     
      case GSSoundFormatPCM32:
        _format.bits = 32;
        break;
     
      case GSSoundFormatFloat32: // Float and double not supported by libao.
      case GSSoundFormatFloat64:
      default:
        DESTROY(self);
        return nil;
    }
 
        if (byteOrder == NS_LittleEndian)
          {
            _format.byte_format = AO_FMT_LITTLE;
    }
  else if (byteOrder == NS_BigEndian)
    {
      _format.byte_format = AO_FMT_BIG;
    }
  else
    {
      _format.byte_format = AO_FMT_NATIVE;
    }
       
        return self
}

- (BOOL)open
{
  dev = ao_open_live(default_driver, &format, NULL);
  return ((dev == NULL) ? NO : YES);
}

- (void)close
{
  ao_close(_dev);
}

- (BOOL)writeBytes: (void *)bytes length: (NSUInteger)length
{
  return ((ao_play(_dev, bytes, (uint_32)length) == 0) ? NO : YES);
}

/* Functionality not supported by libao */
- (void)setVolume: (float)volume
{
  return;
}

- (float)volume
{
  return 1.0;
}

- (void)setPlaybackDeviceIdentifier: (NSString *)playbackDeviceIdentifier
{
  return;
}

- (NSString *)playbackDeviceIdentifier
{
  return nil;
}

- (void)setChannelMapping: (NSArray *)channelMapping
{
  return;
}

- (NSArray *)channelMapping
{
  return nil;
}

@end

---

[SndfileSource.m]

---

/*
   SndfileSource.m

   Load and read sound data using libsndfile.

   Copyright (C) 2009 Free Software Foundation, Inc.

   Written by:  Stefan Bidigaray <stefanbidi@...>
   Date: Jun 2009
   
   This file is part of the GNUstep GUI Library.

   This library is free software; you can redistribute it and/or
   modify it under the terms of the GNU Lesser General Public
   License as published by the Free Software Foundation; either
   version 2 of the License, or (at your option) any later version.

   This library is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
   Lesser General Public License for more details.

   You should have received a copy of the GNU Lesser General Public
   License along with this library; see the file COPYING.LIB.
   If not, see <http://www.gnu.org/licenses/> or write to the
   Free Software Foundation, 51 Franklin Street, Fifth Floor,
   Boston, MA 02110-1301, USA.
*/

#include <Foundation/Foundation.h>
#include <AppKit/NSSound.h>
#include <GNUstepGUI/GSSoundSource.h>
#include <sndfile.h>

@interface SndfileSource : NSObject <GSSoundSource>
{
  NSData *_data;
  SNDFILE *_snd;
  SF_INFO _info;
 
  NSUInteger _curLoc;
  NSTimeInterval _dur;
  int _encoding;
}

- (NSData *)data;
- (NSUInteger)currentPosition;
- (void)setCurrentPosition: (NSUInteger)curPos;
@end

static inline sf_count_t dataLength (void *user_data)
{
  SndfileSource *snd = (SndfileSource *)user_data;
 
  return (sf_count_t)[[snd data] length];
}

static inline sf_count_t dataSeek (sf_count_t offset, int whence, void *user_data)
{
  SndfileSource *snd = (SndfileSource *)user_data;
 
  switch (whence)
    {
      case SEEK_SET:
        [snd setCurrentPosition: (NSUInteger)offset];
        break;
      case SEEK_END:
        offset = (sf_count_t)[[snd data] length] + offset;
        [snd setCurrentPosition: (NSUInteger)offset];
        break;
      case SEEK_CUR:
        [snd setCurrentPosition: (NSUInteger)offset];
        break;
      default:
        return 0;
    }
  return (sf_count_t)[snd currentPosition];
}

static inline sf_count_t dataRead (void *ptr, sf_count_t count, void *user_data)
{
  NSRange range;
  SndfileSource *snd = (SndfileSource *)user_data;
 
  /* Can't read more data that we have available */
  if (([snd currentPosition] + (NSUInteger)count) > [[snd data] length])
    {
      count = (sf_count_t)([[snd data] length] - [snd currentPosition]);
    }
 
  range = NSMakeRange ([snd currentPosition], (NSUInteger)count);
  [[snd data] getBytes: ptr range: range];
 
  return count;
}

static inline sf_count_t dataWrite (const void *ptr, sf_count_t count, void *user_data)
{
  /* No write support */
  return 0;
}

static inline sf_count_t dataTell (void *user_data)
{
  SndfileSource *snd = (SndfileSource *)user_data;
  return (sf_count_t)[snd currentPosition];
}

/* The libsndfile virtual I/O functions */
static SF_VIRTUAL_IO dataIO = { (sf_vio_get_filelen)dataLength,
                                (sf_vio_seek)dataSeek,
                                (sf_vio_read)dataRead,
                                (sf_vio_write)dataWrite,
                                (sf_vio_tell)dataTell };

@implementation SndfileSource

+ (NSArray *)soundUnfilteredFileTypes
{
  return [NSArray arrayWithObjects: @"wav", @"au", @"snd", @"aif", @"aiff", @"aifc",
           @"paf", @"sf", @"voc", @"w64", @"mat", @"mat4", @"mat5", @"pcf", @"xi",
           @"caf", @"sd2", @"iff", @"flac", @"ogg", @"oga", nil];
}
+ (NSArray *)soundUnfilteredTypes
{
  /* FIXME: I'm not sure what the UTI is for all the types above. */
  return [NSArray arrayWithObjects: @"com.microsoft.waveform-audio", @"public.ulaw-audio",
           @"public.aiff-audio", @"public.aifc-audio", @"com.apple.coreaudio-format",
           @"com.digidesign.sd2-audio",
           /* FIXME: are these right? */
           @"org.xiph.flac-audio", @"org.xiph.vorbis-audio", nil];
}
+ (BOOL)canInitWithData: (NSData *)data
{
  return NO;
}

- (void)dealloc
{
  TEST_RELEASE (_data);
  sf_close (_snd);
 
  [super dealloc];
}

- (id)initWithData: (NSData *)data
{
  [self init];
 
  _data = data;
  RETAIN(_data);
 
  /* FIXME: support multiple types */
  _encoding = GSSoundFormatPCM16;
 
  _info.format = 0;
  _snd = sf_open_virtual (dataIO, SFM_READ, &_info, self);
  if (_snd == NULL)
    {
      DESTROY(self);
      return nil;
    }
 
  return self;
}

- (NSUInteger)readBytes: (void *)bytes length: (NSUInteger)length
{
  return (NSUInteger) (sf_read_short (_snd, bytes, (length>>1)));
}

- (NSTimeInterval)duration
{
  return _dur;
}

- (void)setCurrentTime: (NSTimeInterval)currentTime
{
  _curPos = (NSUInteger)(currentTime * (NSTimeInterval)[_data length]);
}
- (NSTimeInterval)currentTime
{
  return ((NSTimeInterval)_curPos / (NSTimeInterval)[_data length]);
}

- (int)encoding
{
  return _encoding;
}

- (NSUInteger)channelCount
{
  return (NSUInteger)info.channels;
}

- (NSUInteger)sampleRate;
{
  return (NSUInteger)info.samplerate;
}

- (NSByteOrder)byteOrder
{
  /* Equivalent to sending native byte order */
  return NS_UnknownByteOrder;
}

- (NSData *)data
{
  return _data;
}

- (NSUInteger)currentPosition
{
  return _curPos;
}

- (void)setCurrentPosition: (NSUInteger)curPos
{
  _curPos = curPos;
}

@end

---

_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Stef Bidi]

On Fri, Jun 19, 2009 at 4:26 AM, Richard Frith-Macdonald <richard@...> wrote:

I would go further and ask what the point of the _private array is?

Well, the only reason I could come up with right now is that I didn't want to have to include GSSoundSink.h and GSSoundSource.h.  The guy writing an application really doesn't care if these two protocols exist, so I figured it was best to hide the fact that I used them, and a NSCondtionLock, all together.

That's why I put this out there before continuing, I wanted to make sure it was a good decision.

By convention all the instance variables are private (that's what the leading underscore in the variable name means), so if all that's required is to inform developers that they should not use the ivars directly, there's no point in having the array, but maybe it would be worth using a @private declaration.

On the other hand, if the idea is to hide the implementation details so that ivar layouts won't need to change with future revisions (something I think we should all be doing), my preference would be to have a single pointer to a structure containing the required data.
eg.

struct  _NSSoundInternal;

@interface NSSound : NSObject <NSCoding, NSCopying>
{              
@private
 struct _NSSoundInternal       *_internal;
}

Then the designated initialiser allocates memory for the structure, clears it, and assigns the pointer when an instance is initialised, and -dealloc frees the memory at the end, and all internal references to ivars just go indirect from that pointer.  The implementation is completely hidden/private and can be changed in subsequent releases without breaking the ABI.

Should I do this instead?  Fact of the matter is, I'm not sure if the way I'm going about this is the best way forward, so chances are it'll eventually change (hence why I created an array of 4 (void *) but am only using 3 pointers).  I would gladly go this route, specially since, like everyone else here, I'm all for ABI compatibility.

Thanks
Stefan

_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[David Chisnall]

On 19 Jun 2009, at 17:30, Stefan Bidigaray wrote:

> Then the designated initialiser allocates memory for the structure,  
> clears it, and assigns the pointer when an instance is initialised,  
> and -dealloc frees the memory at the end, and all internal  
> references to ivars just go indirect from that pointer.  The  
> implementation is completely hidden/private and can be changed in  
> subsequent releases without breaking the ABI.
>
> Should I do this instead?  Fact of the matter is, I'm not sure if  
> the way I'm going about this is the best way forward, so chances are  
> it'll eventually change (hence why I created an array of 4 (void *)  
> but am only using 3 pointers).  I would gladly go this route,  
> specially since, like everyone else here, I'm all for ABI  
> compatibility.

While we're talking about this, has anyone apart from me tested the  
non-fragile ivars support that is now in clang and works with the  
libobjc patch I posted here a month or so ago?  When we have non-
fragile ivars, this problem goes away; just do what Apple does and  
don't declare any ivars in the public headers, then have a version of  
the @interface in the .m file that does.  This works nicely for me,  
but it needs further testing before I push the libobjc diff upstream.

David


_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Stef Bidi]

I'm hoping to start testing this weekend, so I'd like to get some input on some of the things that were brought up last week!  I already know the sink and source objects work correctly (tested them last weekend), and that design is finalized (baring any thing I find I might have to add or remove this weekend).  I also don't have the bundle loading code so far because I want to keep it as simple as I can now to make sure it all works before adding that stuff.

Quickly scanning through the e-mails, this is what was brought up last week:

* Current design - I explained what I was trying to do (not sure if it was understood), so I hope this is no longer a problem.
* IVars - what am supposed to be doing here?  I used that _private array, and Richard mentioned the structure.  Should I be doing something different?

I'll start working on the bundle loading code as soon as I know what I currently have works as expected.

Thanks
Stefan

_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Stef Bidi]

In case you guys didn't know, my weekends start of Fridays (4 day a week work schedule).  I actually started testing last night and, to my suprise, it worked pretty well (I've attached my test tool).  It would be really great to get some input on the code itself.  It's missing quite a bit or functionality but the over all play, stop, etc stuff is working.

Here's what I know isn't working:

* -isPlaying is still kind of weird because I'm using the NSConditionLock's condition to figure out if it's playing but that doesn't get destroyed when the sound naturally reaches the end of the line (would appreciate some suggests on how to make sure this happens).

* -stop destroys the NSConditionLock, but to make sure it doesn't before -_stream is done with it I put a sleep() there.  This is related to the -isPlaying thing above... I'm think the best thing for me to do here is have -_stream destroy the lock, is that a good idea?

* -currentTime and -setCurrentTime are screwy.  I need to go back in SndfileSource.m and have those method use sndfile's tell and seek function.  It's real funny watching .ogg and .flac files play and the output of -currentTime is 1/3 of the actual play time.

* Like I mentioned before, still no bundle loading stuff!  I'm just including SndfileSource.m and AudioOutputSink.m in the NSSound_test.m file... just a real quick and dirty workaround for now.

I also have a question: how can I find out if I have any objects that are not getting deallocated?  In case you didn't catch from my previous stuff I'm pretty bad a memory management.

Thanks
Stefan


_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Stef Bidi]

I know I'm replying a lot to my own posts, but that's just it's me.

Anyway, I've been doing some more testing and the -performSelectorOnMainThread:withObject:waitUntilDone: call in -_stream is not really getting executed, as far as I can tell.  I moved the DESTROY(lock) stuff into -_finished: and was getting a seg fault later down the line.  I set a break point inside -_finished:, but gdb never got to it, which leads me to believe the method is not being called.  I then replaced that call with -performSelector:withObject:, which the way understand simply does the same thing but on the secondary thread, and it worked.  Is -performSelectorOnMainThread:... not working?

Thanks
Stefan

_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Wolfgang Lux]

Am 27.06.2009 um 18:06 schrieb Stefan Bidigaray:

> I know I'm replying a lot to my own posts, but that's just it's me.
>
> Anyway, I've been doing some more testing and the -
> performSelectorOnMainThread:withObject:waitUntilDone: call in -
> _stream is not really getting executed, as far as I can tell.  I  
> moved the DESTROY(lock) stuff into -_finished: and was getting a  
> seg fault later down the line.  I set a break point inside -
> _finished:, but gdb never got to it, which leads me to believe the  
> method is not being called.  I then replaced that call with -
> performSelector:withObject:, which the way understand simply does  
> the same thing but on the secondary thread, and it worked.  Is -
> performSelectorOnMainThread:... not working?

I had a look at the code you posted yesterday and I've found two  
dubious points in NSSound_test.m:

First, there is a retain/release error in NSSound's -
initWithContentsOfFile:byRef: method. The NSData object returned from  
dataWithContentsOfMappedFile: is already autoreleased, but you  
release that object after calling -initWithData: (recall that you  
should only release objects that you own, which in general are only  
objects that you have allocated yourself either with alloc or copy).

Second, I noticed that you call -performSelectorOnMainThread... with  
waitUntilDone = NO. I think you should use waitUntilDone = YES here.  
Note that if waitUntilDone = NO and you call -
performSelectorOnMainThread... from the main thread the call is  
scheduled to be run at the next iteration of the run loop and not  
performed immediately. In addition, if you really use waitUntilDone =  
NO on purpose you should make sure that the sound object is retained  
until the scheduled method is actually run (i.e., you probably should  
retain self before calling performSelectorOnMainThread and  
autorelease self in _finished). Otherwise, there is a chance that the  
thread which calls the _stream method releases the sound object too  
early and the run loop invokes _finished for an already released object.

Wolfgang



_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Wolfgang Lux-3]

> I had a look at the code you posted yesterday and I've found two  
> dubious points in NSSound_test.m:

and here is a third, minor point:

In the -dealloc method of NSSound you check whether self is still an  
element of the dictionary nameDict. This is completely unnecessary  
since the dictionary retains its elements and therefore if a sound  
object is deallocated and the test self == [nameDict  
objectForKey:_name] succeeds this would mean that self has been  
incorrectly released too often.

Wolfgang



_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Stef Bidi]

Thanks Wolfgang, this stuff really helps.

On Sat, Jun 27, 2009 at 4:32 PM, Wolfgang Lux <wolfgang.lux@...> wrote:

I had a look at the code you posted yesterday and I've found two dubious points in NSSound_test.m:

First, there is a retain/release error in NSSound's -initWithContentsOfFile:byRef: method. The NSData object returned from dataWithContentsOfMappedFile: is already autoreleased, but you release that object after calling -initWithData: (recall that you should only release objects that you own, which in general are only objects that you have allocated yourself either with alloc or copy).

I went ahead and made this change.  Is the reason I wasn't getting a seg fault due to the TEST_RELEASE instead of a normal RELEASE?
 

Second, I noticed that you call -performSelectorOnMainThread... with waitUntilDone = NO. I think you should use waitUntilDone = YES here. Note that if waitUntilDone = NO and you call -performSelectorOnMainThread... from the main thread the call is scheduled to be run at the next iteration of the run loop and not performed immediately. In addition, if you really use waitUntilDone = NO on purpose you should make sure that the sound object is retained until the scheduled method is actually run (i.e., you probably should retain self before calling performSelectorOnMainThread and autorelease self in _finished). Otherwise, there is a chance that the thread which calls the _stream method releases the sound object too early and the run loop invokes _finished for an already released object.

I tried setting waitUntilDone to YES, still no luck!  The -_finished method still doesn't get called when calling -performSelectorOnMainThread:..., for now I've just made that a -performSelector:withObject:, which works, but is not what I want done.  I guess my next question here is: is -performSelectorOnMainThread:withObject:waitUntilDone: working?  Maybe I built -base incorrectly?

As for the check in -dealloc, it was inherited from the original implementation, I'll go ahead and remove it.  This probably explains why I used to get a crash whenever -dealloc was called.

On related news.... I've implemented the bundle/plug-in loading code, and so far it works flawlessly.  There's still a lot that needs to be done, but I think I might have this fully implemented by mid July.

I'm also having issues with -setCurrentTime:, I'm now using sf_seek() in SndfileSource.m and the tool will crash either on that call or on sf_read_short() in -_stream.  The best I've come up with is that sf_seek() and sf_read_short() are trying to access the same data, at the same time (sf_seek() is called from the main thread and sf_read_short() from the stream thread) and blowing up.  I thought adding a lock in NSSound's -setCurrentTime: would fix this problem but it did not.  Hopefully I'll get some more stuff done today and will post the latest version of the test tool for you guys to take a look.

Thanks
Stefan

_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Stef Bidi]

As promised, here's the latest code.  Feel free to poke as many holes in it as possible.  I'm going to use this, almost completely unchanged, for NSSound.m.  The bundle/plug-in stuff will go in 100% unchanged.

To build this, you'll need to go into the bundles/ directory to build and install the 2 bundles/plug-ins before hand.  The plug-ins are not required, the NSSound object will simply be destroyed (see -initWithData:) if suitable plug-ins are not found.  There are a few "FIXME" tags in NSSound_test.m, these are there to remind me that I need to fix that before going any further (suggestions are welcome).  The "FIXME" in -soundNamed: however, is not mine and I really do not know what to do there.

Thanks
Stefan


_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Wolfgang Lux]

Stefan Bidigaray wrote:

> I tried setting waitUntilDone to YES, still no luck!  The -
> _finished method still doesn't get called when calling -
> performSelectorOnMainThread:..., for now I've just made that a -
> performSelector:withObject:, which works, but is not what I want  
> done.  I guess my next question here is: is -
> performSelectorOnMainThread:withObject:waitUntilDone: working?  
> Maybe I built -base incorrectly?

I don't think so. The problem with -
performSelectorOnMainThread:withObject:waitUntilDone: is that the  
call is only scheduled to be executed by the run loop of the main  
thread. Thus, the call will not be executed before the next event is  
received or the next timer expires on the main thread. This is indeed  
different from Mac OS X, where the scheduled call seems to be  
executed immediately if the run loop is idle. As a workaround, you  
could schedule a timer that regularly invokes an empty method in your  
NSSound class on the main thread, e.g.,
   [NSTimer scheduledTimerWithTimeInterval: 1.0
                                    target: self
                                  selector: @selector(_heartBeat:)
                                  userInfo: nil
                                   repeats: YES];
and
   - (void)_heartBeat:(NSTimer *) { }

Wolfgang



_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Richard Frith-Macdonald-2]

On 28 Jun 2009, at 21:46, Wolfgang Lux wrote:

> Stefan Bidigaray wrote:
>
>> I tried setting waitUntilDone to YES, still no luck!  The -
>> _finished method still doesn't get called when calling -
>> performSelectorOnMainThread:..., for now I've just made that a -
>> performSelector:withObject:, which works, but is not what I want  
>> done.  I guess my next question here is: is -
>> performSelectorOnMainThread:withObject:waitUntilDone: working?  
>> Maybe I built -base incorrectly?
>
> I don't think so. The problem with -
> performSelectorOnMainThread:withObject:waitUntilDone: is that the  
> call is only scheduled to be executed by the run loop of the main  
> thread. Thus, the call will not be executed before the next event is  
> received or the next timer expires on the main thread.

Not exactly ... calling -
performSelectorOnMainThread:withObject:waitUntilDone:  also triggers  
an event in the run loop of the main thread (by writing to a pipe that  
the thread is listening to), so as the pipe will wake up a sleeping  
thread, the call should be executed pretty much immediately (though  
after any other work already in progress, or waiting to execute) as  
long as the loop is running.

> This is indeed different from Mac OS X, where the scheduled call  
> seems to be executed immediately if the run loop is idle.

OSX is supposed to behave the same way as GNUstep... it should handle  
the perform on the next run loop iteration, so if the loop is not  
being run the perform should be held up  until it is.

Maybe the runloop is not running, or  maybe it's running, but not in  
the mode that was specified for the method to be performed in?



_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Stef Bidi]

On Mon, Jun 29, 2009 at 12:59 AM, Richard Frith-Macdonald <richard@...> wrote:

Maybe the runloop is not running, or  maybe it's running, but not in the mode that was specified for the method to be performed in?

 

This would probably explain it!  I don't have a run loop on that test... it's just a straight shot.  I went back to the Apple doc on it, reread it and realized I completely missed the paragraph that said it queued the message on the run loop!

 

Thanks

Stefan

_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Wolfgang Lux]

Richard Frith-Macdonald wrote:

> On 28 Jun 2009, at 21:46, Wolfgang Lux wrote:
>
>> Stefan Bidigaray wrote:
>>
>>> I tried setting waitUntilDone to YES, still no luck!  The -
>>> _finished method still doesn't get called when calling -
>>> performSelectorOnMainThread:..., for now I've just made that a -
>>> performSelector:withObject:, which works, but is not what I want  
>>> done.  I guess my next question here is: is -
>>> performSelectorOnMainThread:withObject:waitUntilDone: working?  
>>> Maybe I built -base incorrectly?
>>
>> I don't think so. The problem with -
>> performSelectorOnMainThread:withObject:waitUntilDone: is that the  
>> call is only scheduled to be executed by the run loop of the main  
>> thread. Thus, the call will not be executed before the next event  
>> is received or the next timer expires on the main thread.
>
> Not exactly ... calling -
> performSelectorOnMainThread:withObject:waitUntilDone:  also  
> triggers an event in the run loop of the main thread (by writing to  
> a pipe that the thread is listening to), so as the pipe will wake  
> up a sleeping thread, the call should be executed pretty much  
> immediately (though after any other work already in progress, or  
> waiting to execute) as long as the loop is running.

Ah well, I did overlook this when skimming over NSThread.m. So now I  
understand the point of threadInfo->inputFd in GSRunLookCtxt :-)

>> This is indeed different from Mac OS X, where the scheduled call  
>> seems to be executed immediately if the run loop is idle.
>
> OSX is supposed to behave the same way as GNUstep... it should  
> handle the perform on the next run loop iteration, so if the loop  
> is not being run the perform should be held up  until it is.
>
> Maybe the runloop is not running, or  maybe it's running, but not  
> in the mode that was specified for the method to be performed in?

There is a subtle difference between OS X and GNUstep: As far as I  
understand the code, threadInfo->inputFd is not even checked if the  
run loop has no event sources and no timer and it is just added  
implicitly to the event sources in -pollUntil:within: in  
GSRunLoopCtxt. On OS X, on the other hand, (the equivalent of)  
threadInfo->inputFd automatically becomes an input source once the  
thread starts a new thread. So, if you call NSRunLoop's -
runMode:beforeDate: in GNUstep after starting a new thread without  
adding a timer or an input source, it returns immediately with NO and  
it does not perform an invocation scheduled by the secondary thread,  
whereas on OS X -runMode:beforeDate: performs the scheduled  
invocation and then returns YES. If I find time, I'll try to make up  
a test case.

Wolfgang



_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Richard Frith-Macdonald-2]

On 30 Jun 2009, at 07:36, Wolfgang Lux wrote:

> There is a subtle difference between OS X and GNUstep: As far as I  
> understand the code, threadInfo->inputFd is not even checked if the  
> run loop has no event sources and no timer and it is just added  
> implicitly to the event sources in -pollUntil:within: in  
> GSRunLoopCtxt. On OS X, on the other hand, (the equivalent of)  
> threadInfo->inputFd automatically becomes an input source once the  
> thread starts a new thread. So, if you call NSRunLoop's -
> runMode:beforeDate: in GNUstep after starting a new thread without  
> adding a timer or an input source, it returns immediately with NO  
> and it does not perform an invocation scheduled by the secondary  
> thread, whereas on OS X -runMode:beforeDate: performs the scheduled  
> invocation and then returns YES. If I find time, I'll try to make up  
> a test case.

Yes please ... sounds like one of those cases where GNUstep does what  
the documentation says rather than what OSX actually does.  It's  
always good to mirror the OSX  behavior for compatibility (and note  
things in our documentation).  We can probably mimic that behavior  
fairly easily.


_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev

[Richard Frith-Macdonald-2]

On 28 Jun 2009, at 19:14, Stefan Bidigaray wrote:

> As promised, here's the latest code.  Feel free to poke as many  
> holes in it as possible.  I'm going to use this, almost completely  
> unchanged, for NSSound.m.  The bundle/plug-in stuff will go in 100%  
> unchanged.

Sorry that I just haven't found time to look at this.

But ... going back to the issue of avoiding changes to ivars breaking  
ABI in future releases ... the approach I currently favor is having a  
*single* ivar in the public class.  This is a private id variable  
referring to an instance of a private class which is used to hold the  
real ivars.

So the implementation file contains this private class (whose ivars  
are all declared public so that any code in the implementation file  
can access them freely), and the -init and -dealloc methods of the  
public class create/destroy an instance of the private class.  Look at  
the code for NSOperation.m in the current svn trunk version of the  
base library for the simplest example around.





_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@...
http://lists.gnu.org/mailman/listinfo/gnustep-dev