[Bug 642690] [baseaudio] GstBaseAudioEncoder and GstBaseAudioDecoder class

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=642690
  GStreamer | gst-plugins-base | git

--- Comment #12 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2011-08-12 17:25:45 UTC ---
Some niggles:

 === baseaudioutils ===

 - should all be folded into audio.c (without the
   _base_ stuff) now that we're moving it into
   libgstaudio directly

 - GstAudioState isn't really a state, more like
   an audio format description

 - GstAudioState: some overlap with GstRingBufferSpec
   and the (slightly misnamed) GstBufferFormat and the
   buffer_parse_caps stuff. Not quite sure what follows
   from that, it just feels like we could do something
   better here.

 - GstAudioState: needs new/free or init/clear functions,
   since channel positions are g_malloc()ed.

 - _parse_caps(): is the changed parameter widely-used
    here? Feels cleaner to separate parsed + compare.

 - GTypes for the structs (as Sebastian already said)


 === baseaudioencoder ===

 - GstBaseAudioEncoderContext "deriving" from
    GstAudioState seems a bit weird - are they really
    related? It's going to be awkward for bindings,
    because if we just register boxed types we can't
    derive from another struct / boxed type.

 - in fact, I'm not sure I see the point of the
   Context struct at all - what does it buy us?

   It's not used in any part of the API directly, so
   it doesn't serve to ensure future extensibility
   like in the case of GstBaseParseFrame or
   GstVideoFrame. Seems to me that maybe
   we should just get rid of it and add
   _set_foo_bar() API for the various fields
   instead, to be used by the subclasses?

 - add setters+getters for the properties?

 - "granulepos" - maybe we can come up with
    a better property name? $verb-granulepos
    or so instead of $noun :)

 - "perfect-ts":  would perfect-timestamp[s]
   or "perfect-time" be better maybe? The
   rtp payloaders use perfect-rtptime and
   identity has check-perfect-timestamp,
   for comparison.

 - "tolerance": encodebin has "audio-jitter-tolerance",
   which is long, but more descriptive. Though the audio
   is somewhat implied here I guess. audiorate also
   uses "tolerance" of course. oggmux uses "max-tolerance".

 - the event vfunc returns a boolean, which makes it
   hard to differentiate between FALSE = "drop event"
   and "not handled, do with it what you want" or
   "handled, but return FALSE". Maybe subclass should
   be required to chain up to parent class event func
   explicitly to support these variations? (But then I
   guess it's not so important if you haven't found a 
   need for that during your plugin porting so far).


 === baseaudiodecoder ===

 - same comment as above regarding
    GstBaseAudioDecoderContext

 - "plc" => "packet-loss-concealment" ?

 - explicit getters/setters for properties?

 - re. event vfunc see above

 - re. "tolerance" see above

 - "latency" - if this is a minimum, maybe
   name it as such? (dunno)


I'm happy to help re-shuffle things if needed/desired.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.