Created attachment 105986 [details] [review]
initial implementation

Features:
  * PUSH & PULL mode
  * SEEK in both modes
  * POSITION/DURATION/SEEKING/CONVERT/FORMATS query
  * FLUSH/NEWSEGMENT event handling
  * Very basic documentation

What is missing:
  * Accurate seek support
  * Indexing (GstIndex stuff)
  * Non-flushing seeks are not tested
  * Use of find_frame is missing

Most probably a lot of changes are needed into this class when other parsers will be changed to use this.

Issues:
  * Src & sink pad handling + their caps. Is it OK to leave these to
    be handled almost entirely by the subclass? Currently this issue seems to
    work with AMR parser, but I am not sure if this is implemented The Right
    Way.

  * Accurate seek. I haven't had time to think about this yet, so maybe
    the seek mechanism needs modifications in order to support accurate seeks.

Some comments from a short look:
- as the newsegment event is pushed just before the first buffer we have to queue up all events (except EOS) and push them after newsegment. This is needed a decodebin only links pads after newsegment and otherwise we loose some events, like tags for example. See my latest change to mp3parse/mad.

- g_object_unref (adapter) already clears the adapter ;)

- the event() methods have no way of telling that the event was handled but an error happened when pushing downstream for example

- there could be some default timestamp tracking for buffers, i.e. look at the upstream newsegment events for start timestamp, if parse_frame() gives a duration calculate everything else with it. This should only be done if parse_frame() doesn't give a valid timestamp!

- IMHO we can pass read-only buffers to check_valid_frame(). This way we don't need to memcpy(), etc.

- Frames that end before segment start and frames that start after segment stop should be dropped but there should be a way to override the dropping behaviour. I.e. for mp3parse we need to start pushing frames a few frames (given as timestamp) before segment start to get decoding from the position we want.

- For the chain function we should update the minimal frame size after every pushed frame. This is to allow things like a special header at the start of the file: the subclass would first have a minimal frame size that is good enough for that header and update it after the header to the real frame size.

- We could add caching for pull mode as is done in matroskademux. Pull a large buffer and return subbuffers of this until we need to pull the next large buffer.

- If the duration is set we should post a duration message (maybe also a tag?)

- For duration/position query we can also answer in BYTES (at least in pull mode) and could also query upstream if everything fails

- The SEEKING query doesn't return something useful in push mode although seeking should be supported there too.

- When seeking we currently fail if the subclass can't convert.  This is of course not optimal, we could simply run chain until we're in the seek segment or we could simply pull until then. The same could be done for accurate seeks too (if subclass can't do an accurate conversion... need some flag for this).

Hm, for find_frame() it would be nice to have a gst_base_parse_add_seek_entry() function so base parse can populate it's index.

I'd propose to use a simple binary tree for the seek table, sorted by timestamp (which should in theory sort it by offset too). Always using GstIndex is not a good idea because of the overhead it imposes, IMHO it should only be used if an index is set on the element.

And one could do, even if the subclass can't convert, conversions between the formats by using the average bitrate. This could also be used by seeking if it's not meant to be accurate.

Created attachment 106082 [details] [review]
baseparse-2.diff

- Make buffer given to check_valid_frame() read only
- cache events until newsegment is sent (FIXME: which should be sent immediately?)
- Drop frames outside the configured segment
- Set last_stop always and at a place where the buffer is still valid
- Update min_frame_size on every frame
- Post duration message if duration changes
- Be a bit more intelligent with the POSITION, DURATION, SEEKING queries

More to come tomorrow...

Created attachment 108416 [details] [review]
incremental update

here is an INCREMENTAL patch for the GstBaseParse. It needs the earlier patch (that adds the GstBaseParse into GStreamer) to be applied first.

This patch contains following changes:

* Removed one misplaced (?) g_mutex_unlock() in GST_QUERY_DURATION handler
* Added is_seekable vmethod in BaseClass API. This is for SEEKING query
* Added mechanism to drain the GstAdapter when EOS arrives
* Now it is able to handle queries which format == GST_FORMAT_BYTES
* Updated documentation a bit
* White space cleanups

Created attachment 109471 [details] [review]
incremental update 2

here is the second incremental patch. It needs *both* earlier patches to be applied first.

This patch fixes following issues:

* API change: the "skip_size" is now signed, and default value is -1. 
  This way the subclass can tell parent class not to skip any bytes, by
  setting the skip_size = 0. This can happen e.g. after DISCONT, when subclass
  might want to get more data.

* Mark the buffers sent to check_valid_frame() with DISCONT flag as well,
  when needed. This way subclass data parser can know out-of-sync
  situations in frame-checking phase already (and e.g. ask for more data
  if needed).

* Allow the min_frame_size changes during a parsing loop (PULL mode).

* Documentation updates

Created attachment 109805 [details] [review]
merging the diffs

Created attachment 117744 [details]
gstbaseparse.c

My latest version of gstbaseparse.c with all the diffs here merged adjusted for flac (GstFlacBaseParse for the GType name). This fixes some bugs and improves things at some places...

- Add a GstBaseParseClassPrivate for future extensions and add padding to structs
- In pull mode, pull larger chunks and consume them to prevent a million 1-byte pulls
- Always set the pad caps on outgoing buffers
- don't cache GST_EVENT_FLUSH_START
- Fix some timestamp calculations to not do calculations with GST_CLOCK_TIME_NONE
- In pull mode, post an initial NEWSEGMENT event
- In pull mode, if the frame size is larger than the min_frame_size, pull the complete frame instead of passing not enough data to the subclass


There are many more issues and possible improvements but I'll care for that later ;)

Created attachment 117746 [details]
gstbaseparse.h

Hm, we also need to queue up all parsed frames until the subclass has set the caps on the pad, otherwise things will break ;) This is needed for FLAC for example... I can only set the caps after all header buffers (>=3) are parsed (because of the stream_headers caps field).

Created attachment 118746 [details] [review]
aac parser using the baseparse class (patch for gst-plugins-bad)

Created attachment 118747 [details] [review]
amr parsers using the baseparse class (patch for gst-plugins-bad)

Created attachment 119482 [details]
gstbaseparse.c

Some smaller bugfixes and updated TODO list

Created attachment 119483 [details]
gstbaseparse.h

Created attachment 120027 [details] [review]
amr parsers using the baseparse class (patch for gst-plugins-bad)

Fix BaseParse name to avoid nameing clash.

Created attachment 121953 [details] [review]
aac parser using the baseparse class (patch for gst-plugins-bad)

Fix Makefile.am. Update baseclasses.

Created attachment 121954 [details] [review]
amr parsers using the baseparse class (patch for gst-plugins-bad)

Fix Makefile.am. Update baseclass

This is all in gst-plugins-bad now.

Re-opening this for discussion of final (cosmetic) API changes, and because we want this in core, not -bad.

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?

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

 - gst_base_parse_set_frame_props() -> gst_base_parse_set_frame_properties()?

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

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

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

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

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

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

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

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

On further reflection, GstBaseParse::segment might be useful for custom clipping cases or so ...

Some notes:
- GST_BASE_PARSE_SEGMENT() should probably return a pointer instead of the segment. And similar to basesrc/basesink/basetransform it probably makes sense to expose it. For clipping, QoS, $magic
- Since markers are 0.10.x, should be 0.10.33
- GST_BASE_PARSE_FORMAT_PASSTHROUGH talks about pull mode but it only makes sense in push mode
- adapter, pending/close segment should really be private IMHO
- I have to agree with Tim about the seeking, just make this a SYNCABLE flag and a average bitrate setting function

Ok, mostly done with this I think.

Still need to fix up the DRAIN and SYNC macros though. On IRC we talked about just making them flags on GstBaseParse (instead of the frame). That would work for me, but I wonder if just adding gbooleans wouldn't be more straight-forward (all this just to avoid the overhead of one or two function calls to per frame - is that really worth it?).

Thing is that we'll probably need to add _get_type() functions etc. for all the different flags, and we'll likely also need to register a boxed type for GstBaseParseFrame, for bindings (that's going to be awkward enough.

For the format flags I'm also wondering (again) if doing set_passthrough(), set_syncable() etc. wouldn't be nicer and more straight-forward after all.

Added and changed things to (hopefully) make GstBaseParseFrame use possibly from bindings.

That's it for me, closing.

(PS: yay!)