GNOME Bugzilla – Bug 650093
[baseparse] GstBaseParseFrame extensibility/flexibility needed
Last modified: 2013-07-24 07:51:50 UTC
As it stands currently, GstBaseParseFrame is a pretty boxed/static/independent type, whereas actually more something like GstCollectData relative to GstCollectPads would be useful/needed. In particular, subclass should be able to store some parsing state of its own in a frame which it can use across subsequent invocations. This is particularly useful/needed for e.g. current h264parse which keeps (a.o.) track of last start code position (to resume scanning from that mark). This is currently tracked globally in h264parse itself and usually works, but it may/will actually go wrong if some seek or otherwise repositioning occurs that disturbs the expected callback sequence and leads to parsing new data with stale state (e.g. last scan mark). This might be handled by allowing to register an extended GstBaseParseFrame size along with some DestroyNotify, InitNotify and/or CopyNotify for a GstBaseParseFrame, but that would lead to a GstBaseParseFrame with "multiple faces". Failing such to be possible, quite some typical subclass cases (e.g. h264parse) would end up with a lot more complicated (and less efficient) parsing code that would have to repeatedly start from scratch and parse all over again.
We could replace one of the reserved pointers of GstBaseParseFrame with a pointer to element specific data... and make it a miniobject or GValue to allow copying and be able to free it later.
Presumably it is then up to subclass to actually fill in this pointer and set up corresponding area, in which case that is one approach. A simplified version would be a flag (e.g.) GST_BASE_PARSE_FRAME_FLAG_SEEN_FRAME, that could be set by subclass whenever it stores some state data, and this flag not being set is a sign to clear any state data (in the case above, pointer == NULL then serves as such as a "sign"). The former is more general/cleaner, the simpler approach entails less bloat for the subclass and might avoid some additional alloc/free.
In any case, we would have to maintain frame state during a particular parsing round, which mainly means keeping a GstBaseParseFrame around during subsequent _chain calls (whereas the pull mode path does not present a problem this way). Following commit at least goes this part of the way: commit 35567592efcbd44267048f006c0b910bb3d2b5ca Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Tue May 17 22:17:14 2011 +0200 baseparse: maintain frame state during frame parsing round See #650093.
> In any case, we would have to maintain frame state during a particular parsing > round, which mainly means keeping a GstBaseParseFrame around during subsequent > _chain calls (whereas the pull mode path does not present a problem this way). > > Following commit at least goes this part of the way: > > commit 35567592efcbd44267048f006c0b910bb3d2b5ca > Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> > Date: Tue May 17 22:17:14 2011 +0200 > > baseparse: maintain frame state during frame parsing round Sorry, completely missed that commit. Right, so that takes care of part 1. Now the next step would be to allocate this frame dynamically on the heap instead of embedding it into the struct, no? And then somehow provide API for the sub-class to tell us how many bytes to allocate for that struct and/or custom alloc/free funcs or so.
Something for 0.11 perhaps, if we can't be bothered to implement this for 0.10 ?
Mark, can this be closed or do we still want it?
Looks like we have managed so far without it, so presumably that will do unless some other/more necessity would show up.
Shall we close this then ?
I guess so, nobody complained yet :)