[gstreamer-bugs] [Bug 597616] [plugin-move] move mimic to gst-plugins-ugly

[GStreamer (bugzilla.gnome.org)]

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

Tim-Philipp Müller <t.i.m> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
                 CC|                            |t.i.m at zen.co.uk
     Ever Confirmed|0                           |1

--- Comment #2 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2009-10-10 17:22:34 UTC ---
Ok, so here's my take:

General:
• why are the elements called mim{dec,enc} and not mimic{enc,dec}? I think they
should be renamed
• gst_mimenc_xyz -> gst_mim_enc_xyz() and GST_MIMENC -> GST_MIM_ENC to match
the type GstMimEnc
• should use GST_DEBUG_FUNCPTR where appropriate
• Makefile.am needs fixing (order of CFLAGS + LIBS) 

Encoder:
• documentation is a bit sparse, the paused-mode thing should be mentioned
• paused-mode: I don't think this is how this kind of functionality should be
implemented (as mentioned on IRC a while back); IMHO it shouldn't be
implemented in the encoder, but in an upstream element (maybe a dedicated one?)
or the app, which then sends some kind of event downstream that the encoder can
then use to generate fill frames.
• where does the '4s' come from for paused mode? Should this be configurable as
well?
• warn if paused-mode property is changed in PLAYING state?
• locking looks a bit weird - most things should be protected by the streaming
lock, additional GST_OBJECT_LOCKs on top of that shouldn't be needed (state
change function etc.) (I guess it's done this way because of the separate
paused-mode task?)
• this code needs fixing (in paused_mode_task()):
    ret = gst_pad_push (mimenc->srcpad, buffer);
    if (ret < 0) {
• GST_MIMENC_CLASS and GST_IS_MIMENC_CLASS macros are broken
• should MAX_INTERFRAMES be configurable at runtime?
• should use GST_ELEMENT_ERROR() when returning GST_FLOW_ERROR (e.g. in
_chain()), and use the goto xyz_failed; style used elsewhere in gstreamer (ie.
put error stuff at end of function)
• don't use c++ style comments
• gst_mimenc_create_tcp_header() can't fail given its current implementation,
so no need to handle a NULL return really
• combine the if(event) branch in the chain function with the need_newsegment
if?
• why alloc separate GstBuffers for fixed header + encoded buffer instead of
allocing buffer_size+24 and then passing data+24 to mimic_encode_frame()?
• newsegment event handling and GstSegment usage looks generally dodgy
• not sure if using GST_ELEMENT_CLOCK() macro in paused_mode_task() is 100%
kosher - the element clock is protected by the object lock, while the pad task
will take the streaming lock.
• it seems to me like there is common code between the paused mode task and the
change function, so some refactoring would be nice here
• GstElementDetails need fixing (first+third string, variable name, use
_set_simple())
• encoder context open/close should be done in state change func imho
• does not set/unset DELTA_UNIT flag on output buffers to mark keyframes and
delta-frames

Decoder:
• GST_MIMDEC_CLASS and GST_IS_MIMDEC_CLASS macros are broken
• src_factory caps: width/height are not really accurate, are they? Isn't the
codec limited to two resolutions? framerates? (tighter caps allow to detect
problems at link time already)
• GstElementDetails need fixing (first+third string, variable name, use
_set_simple())
• chain_function: 'buf' variable unnecessary
• should probably handle DISCONT flag on incoming buffers?
• chain_function: adapter usage is a bit weird, and local state is kept in
instance variables (payload_size, have_header): function should do a peek for
the header, then retrieve the payload size, then make sure there's enough data
for header+frame and process both in one go or wait for more data
• decoder context open/close should be done in state change func imho
• should use GST_ELEMENT_ERROR() when returning GST_FLOW_ERROR (e.g. in
_chain()), and use the goto xyz_failed; style used elsewhere in gstreamer (ie.
put error stuff at end of function)
• maybe use pad_alloc_buffer for the output buffers so no decoding is done if
downstream is unlinked (and bails out faster on flushing etc.)? (guess it
doesn't matter so much here since the codec is not used in files, so there's
not much seeking etc. going on)
• Is the output framerate really always 7fps? (as put on output caps in chain
func) if yes, should fix template caps, if not fix output caps in chain func
• locking is a bit weird - most if not all things should be protected by the
streaming lock - no need for the object lock really
• again: c++ comments
• newsegment handling looks a bit weird


Most of this is not particularly critical or easy enough to fix up.

However, I do dislike the paused-mode thing and the segment handling a lot and
would like more opinions on whether this is how we want to handle this sort of
thing, ie. if that's what we want in a good/ugly plugin or not. If I had to
make a decision right now, I'd punt this for next time.

Other opinions?

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