[Bug 518857] [API] GstBaseParse: new base class for parsers

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=518857
  GStreamer | gstreamer (core) | git

--- Comment #23 from Mark Nauwelaerts <mnauw at users.sourceforge.net> 2011-03-13 21:35:35 UTC ---
(In reply to comment #22)
> So, I've basically moved this into libgstbase in my local core, and fixed up a
> few minor things up here and there.
> 
> I noticed a few cosmetic issues when reviewing the API (let's just try to
> decide what we want to do and I'll fix it up locally):
> 
>  - gst_base_parse_set_seek() => rename to ???
>    (set_seekable, set_seekability, set_seek_type, set_seek_mode?)
> 
>  - or maybe get rid of gst_base_parse_set_seek() entirely, and just
>    have _set_bitrate()? Or add a separate _set_average_bitrate()?
> 
>  - GstBaseParseSeekable => GstBaseParseSeekability or GstBaseParseSeekMode?
>    (see above)
> 
>  - Name mismatch with "GstBaseParse*Seekable*" and
> GST_BASE_PARSE_*SEEK*_DEFAULT
>    (should be renamed according to whatever is decided above)
> 
>  - GST_BASE_PARSE_SEEK*_DEFAULT => GST_BASE_PARSE_SEEK*_BITRATE?

Seems like we could get rid of seek* entirely and stick to a
set_(average_)bitrate (or set_bitrates allowing for min/max as well?).

> 
>  - gst_base_parse_set_format() looks a bit weird API-wise ("gboolean on"),
> maybe
>    make it {set|unset}_format_flag() or something like that?

I'd go for set_format_flags (with a gboolean on), but taste varies.

> 
>  - gst_base_parse_set_frame_props() -> gst_base_parse_set_frame_properties()?
> 
OK.

>  - should we add some padding to GstBaseParseFrame? (better safe than sorry
> etc.)

Probably.  The idea/was is for this not to be needed by having
GstBaseParseFrame grow as needed (without interfering with existing code).
Only flacparse using gst_base_parse_frame_init (and _clear done in baseparse)
is complicating that approach, which might be alleviated by using a _new/_free
instead (though a repeated alloc/free is also preferably avoided).

> 
>  - GstBaseParseFrame: shouldn't flags be GstBaseParseFrameFlags instead of
> guint
OK

> 
>  - does GstBaseParse::segment need to be exposed? (nothing uses it afaict)
> 
>  - does GstBaseParse::pending_segment need to be exposed? (nothing uses it
> afaict)
> 
>  - does GstBaseParse::close_segment need to be exposed? (nothing uses it
> afaict)
> 
>  - does GstBaseParse::adapter need to be exposed? (nothing uses it afaict)
> 
Likely not (legacy leftover).

>  - GST_BASE_PARSE_{SRC,SINK}_NAME -> unused, let's remove it (seems pointless
> too)
OK.

> 
>  - GST_BASE_PARSE_FRAME_SYNC() -> rename to express booleaness?
>    (maybe easier if we make it negative, like LOST_SYNC)
> 
>  - GST_BASE_PARSE_FRAME_DRAIN() -> rename to express booleaness?
> ({IS?}_DRAINING?)
> 
OK

>  - GST_BASE_PARSE_FRAME_{DRAIN,SYNC} - shouldn't "draining" and "in sync"
> status
>    be something on GstBaseParse rather than GstBaseParseFrame? (maybe add API
> to
>    get status for those?)

Possibly.  I am not particularly fond of a whole plethora of "little"
_get/_set, and the baseaudioencoder/*decoder approach tends to use a "context"
to side-step that (and avoid additional call(s) each parsing round in as much
that might have impact), but taste may vary (again).

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