I'm out of time to check out the plugin, and going away for the weekend. Someone needs to confirm that it meets the plugin movement checklist: 

http://cgit.freedesktop.org/gstreamer/gstreamer/tree/docs/random/moving-plugins

If so, then move it:

http://git.collabora.co.uk/?p=user/edward/gst-git-migration;a=blob_plain;f=plugin-move.txt;hb=HEAD

Marking as a blocker bug, until a decision whether to move or not is made...

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?

Thanks for the thorough review, I will fix most of these.

I'm not a big fan of the paused-mode thing either, but I have no better idea on how to do it. What is the difference in having a separate element that sends an event every X sec when it has no incoming data compared to having the encoder doing it? Keeping in mind that I know only one user of this codec: the MSN Messenger webcam functionality. That said, you can also just write the whole encoded stream to a file and feed it into the decoder and it will work (but seeking for that is not implemented).

* The 4 second is how the codec works (ie, how MSN does it).
* The slightly strange locking is because of the paused thread

I also had a quick look:

* the encoder has quite fixed template caps, while the decoder has not. Is that intentional?

* decoder: unrefs are done in dispose (to eventualy break ref cycles), frees are done in finalize. Its not an issue here. If you change it, be aware that dispose could be called multiple times (that is set adapter to NULL and check for it).

Sounds like we should hold off on the plugin move this time around and aim to get everything cleaned up for the next cycle.

Removing the blocker tag, so I can make the first pre-release.

(In reply to comment #2)
> Ok, so here's my take:

I fixed most of your concerns in this branch to be merged post-freeze: http://git.collabora.co.uk/?p=user/tester/gst-plugins-bad.git;a=shortlog;h=ref/heads/mimic

For the rest, I have some more questions:

> General:
> • why are the elements called mim{dec,enc} and not mimic{enc,dec}? I think they should be renamed

For historical reasons mostly and compatibility with already deployed Farsight

> Encoder:
> • 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.

I think the proper way to do it is probably to split the encoder/decoder code from the "payloader". And the payloader element is the one that should have the paused-mode thing. But in the end, I didn't implement it because really the only use case for this element is interoperating with the MSN clients and so I didn't really find any use case for separated elements.

> • where does the '4s' come from for paused mode? Should this be configurable as
> well?

That's what MSN does.. and since this is for interop.

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

Actually, the locking was added a bit too aggressively before doing the paused-mode thing, but once that other thread is there, I guess we need it.

> • this code needs fixing (in paused_mode_task()):
>     ret = gst_pad_push (mimenc->srcpad, buffer);
>     if (ret < 0) {

What's wrong here ?

> • GST_MIMENC_CLASS and GST_IS_MIMENC_CLASS macros are broken

What is wrong with GST_IS_MIM_ENC/DEC_CLASS() ?

> • should MAX_INTERFRAMES be configurable at runtime?

Again, its only for interop with a single client, but I guess it would be.

> • combine the if(event) branch in the chain function with the need_newsegment
> if?

There is a unlock in th middle.. otherwise we end up with "if(need_newsegment) {stuff;unlock();} else { unlock(); }"  which I find uglier

> • newsegment event handling and GstSegment usage looks generally dodgy

After more than 2.5 years, I'm still very confused on how the segments work.
What is wrong? And how should it work ?

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

Now holding the lock all over..

> Decoder:
> • should probably handle DISCONT flag on incoming buffers?

What should we do with them ? Flush everything before them ?

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

In theory it could be used on files (amsn does logs of video calls by dumping everythin in a file).. But, it seems that "keyframes" are not real keyframes and and there are broken blocks after a seek. Possibly it is a bug in the decoder.

> • newsegment handling looks a bit weird

Again, I have no idea what I should do. Keeping in mind that it should work both on live streams and on dumps of a live stream to a file.

I updated the mimic branch with everything. The only things I'm not sure is correct is handling of segments and disconts. Any advice on what I should change there ?

Otherwise, I'd like to push it into -ugly next cycle.

After discussion with wtay, I believe the segment handling is now correct (please confirm). I also think that incoming DISCONT won't work, the codec will give out garbage output if the input frames don't all come in order (ie, you can't drop input frames).

So, I'd like to submit ext/mimic for a move to -ugly in this cycle.

I added a few more patches to mimic, I believe this is now ready to move.. Anyone care to re-review ?

This would also have to be ported to the base audio encoder/decoder classes.

Ten years later, does anything still use this codec? Maybe we should just remove it?

IIRC that was for MSN A/V calls isn't it ? (which is long time dead now).

I think so.

Confirmed. This was used for the old MSN webcam function (video only - IIRC A/V calls were using something else). Some people might still want to play back their old aMSN webcam logs, but... they should have just converted them IMHO.

On the other hand, it's not big, it's not causing any bugs, and there could still be valid use cases, so now that I'm re-thinking about it, why not just keep it?

Well, it's a technical liability and has the potential to be the cause of bugs, e.g. someone could craft a file that exploits unmaintained bitrotten code paths.

Also, libav has a decoder for this format now, so if decoding old files is the only concern, this plugin is not needed for that.

In any case, I don't think we're going to move this to -ugly.

If we can in addition to that get consensus to remove it, even better :)

Good point about bitrotting, and also good point about libav. OK, I'd say "remove it" then :)

commit ad661999ad2c79a5d7721f0eb1e7f1f83a304224
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Wed Nov 23 10:31:29 2016 +0000

    mimic: remove ancient codec
    
    This was used by MSN messenger in prehistoric times, it's safe
    to say no one needs or wants this any more these days. For
    decoding old recordings there's still a decoder in ffmpeg.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=597616