[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 #13 from Sebastian Dröge <slomo at circular-chaos.org> 2011-08-15 06:19:31 UTC ---
I've to agree with all of Tim's comments

(In reply to comment #12)

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

Let's call it GstAudioFormatInfo, like in 0.11 GstVideoFormatInfo. And the same
for GstVideoState in the video base class. This way we might require less
changes when moving to 0.11.

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

It's not strictly deriving but yes, this will confuse bindings and make
everything harder for bindings. Better store a pointer to the state.

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

Me neither, I think the context can go away too

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

That's how we do it in the other base classes. It makes sense for consistency
but has the problem you mentioned and also is not nice for bindings. We should
probably keep it for 0.10 this way and for 0.11 change it (with the event
functions returning GstFlowReturn this helps already), especially the event
ownership depending on the return value is a problem.

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