[Bug 642690] [baseaudioencoder] GstBaseAudioEncoder class

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #3 from Mark Nauwelaerts <mnauw at users.sourceforge.net> 2011-03-21 11:52:42 UTC ---
Updated work now in [*] as it includes both baseaudioencoder and
baseaudioencoder, along with common/utils part.

[*]
http://git.collabora.co.uk/?p=user/manauw/gst-plugins-bad.git;a=shortlog;h=refs/heads/baseaudio

(In reply to comment #1)
> Cool stuff! A patch set for the basecalsses would be easier to review though. I
> hope you can match the context.

Not entirely sure what is meant here.  If by that you mean patches for the
ported elements, that is tricky stuff.  As baseclass is in -bad for now, it
would be tricky to have branches all over the various repos, so the plan/hope
is to have the ports in -bad for now, and when baseclasses make it up (to
-base, albeit possibly with experimental marking), then these ports can be
"slammed" (with caution) onto the elements there (thereby turning into a
patch).  In particular, that should be possible for most of these elements, as
the behaviour of the ports is arranged to match (practically identically) the
original element (give or take rarely what would otherwise be fixes to the
elements).  Roughly, only the ported mad is a bit distinct from the original
one, as it no longer minds about tags etc (that belonging to parser).

That all being said, it's possible to first commit a verbatim copy in -bad
audiocodecs, and then modify/port that copy (which should give more of an idea
of the eventual patch in the origin repo) --> next update round.

> 
> vorbiscommon seems to be only used by vorbis.
> 
> gstbaseaudioencoder.h
>  101  * GstAudioState:
> this needs a one liner describing the purpose below the fields. Is it really a
> state or GstAudioFormat. Should that go to gst-plugin-base/libs/audio (could
> ev. be used in the GstRingBuffer too).

Jury/taste still out on this one.  video counterparts have a more elaborate
state here.  In practice, this kind of "state" suffices (and seems more
convenient by itself as a "format").

> 
>  102  * @xint: whether sample data is int or float
> "is_int" looks better to me.
Fixed.

> 
>  178  * GstBaseAudioEncoderClass:
>  200  * @pre_push:       Optional.
> not clear when one would need it. The ported encoders don't seem to use it.

Indeed they do not.  However, baseparse practice has shown it might become
useful, and iirc there have been a few (porting) times it has come close to
using it (or might be advisable/cleaner to do so).  For instance, it might come
in handy having to do semi-hacks in the wavpack(enc) case when having to deal
with the extra srcpad there.

> 
> gstbaseaudioencoder.c
>  104  * <link linkend="GstBaseAudioEncoder--perfect-ts">perfect-ts</link>.
>  108  * than <link linkend="GstBaseAudioEncoder--tolerance">tolerance</link>,
>  116  * (see <link
> linkend="GstBaseAudioEncoder--hard-resync">hard-resync</link>)
> you can write "GstBaseAudioEncoder:perfect-ts" "GstBaseAudioEncoder:tolerance"
> and so on
Fixed.

> 
>  231 GType
>  232 gst_base_audio_encoder_get_type (void)
> shoudl this be using G_DEFINE_ABSTRACT_TYPE_WITH_CODE() ?
Possibly, but current approach allows more coding freedom if needed.

> 
>  310       g_param_spec_boolean ("perfect-ts", "perfect-ts",
> in the name we can spell it out more "perfect timestamps". Also on other
> properties.
Done.

> 
> The whole granule business might need some more explanation. People who don't
> know ogg might not be familiar with it.
Some documentation added.

> 
>  431 /** gst_base_audio_encoder_finish_frame:
> the gtk-doc syntax is (several occurrences):
>  /**
>   * gst_base_audio_encoder_finish_frame:
Fixed.

> 
> 
>  451  * Returns: a #GstFlowReturn that should be escalated to caller (of
> caller)
> typo at the end of the line?
It is really meant that way 
(caller == typically subclass, caller of caller == typically baseclass,
which will then return upstream etc)

> 
>  600     // TODO return value ?
> ha - its not only me doing that (c++ comments) :) (there are more of them)
Fixed.

> 
>  907 #define CHECK_VALUE(res, var, val) \
> Use G_STMT_START/END, in the code below you create several ;; right now - just
> to avoid suprises if some one adds an if() there later on. Also either drop
> 'res' from the args (and use it implicitely) or also pass 'changed' :)
Fixed and moved to utils part.

> 
> 1181   enc = GST_BASE_AUDIO_ENCODER (gst_pad_get_parent (pad));
> event handlers are protected by the streaming lock and thus the pad cannot
> disappear. enc = GST_BASE_AUDIO_ENCODER (GST_PAD_PARENT (pad)); should be
> enough.
Fixed.

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