[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

--- Comment #6 from Olivier Crete (Tester) <tester at tester.ca> 2009-10-13 16:30:38 UTC ---
(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.

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