GNOME Bugzilla – Bug 642671
fieldanalysis: New element for analysing video input to produce progressive output
Last modified: 2011-03-04 07:13:30 UTC
I have been working on an element to analyse any incoming video buffers to identify whether they are progressive, interlaced or telecined and which fields to use from them and how. The decisions are based on various metrics with corresponding thresholds. The default metrics seemed to be the best choice across the limited telecine samples I have. The analysis consists of five comparisons: 1. top vs bottom fields of the current buffer 2. top of current vs top of previous 3. bottom of current vs bottom of previous 4. top of current vs bottom of previous 5. bottom of current vs top of previous This creates two classes of required metrics: same parity (2,3) and opposing parity (1,4,5). It also introduces a one-frame delay to the video branch of the pipeline. Based on the semantics of scores from the above comparisons and using the assumption that same parity comparisons 2 and 3 are likely more reliable (they're often mathematically identical or just have quantisation noise/compression artifacts) I formulated some logic to identify the various different states and apply flags such that downstream deinterlace can act upon the identified states to produce progressive output. It seems to work quite well Known flaws: The thresholds will need manually adjusting for different sources. They are not dynamic to the source. This is something that I would like to have in the element but it will take some more research and design and more time. I would rather have this base work completed for now. I actually found that in practice, often only minor tweaks are required. I will try to put together some guidelines for the tweaking as a stopgap until some more automagicness can be done to make the thresholds adjust according to the current state of the incoming buffers. (Note to self: gradients and perhaps variance of fields or moving averages thereof.) The windowed comb method is supposed to be the most reliable and flexible but in practice I found it very difficult to tweak the thresholds to achieve the desired result and as such not user-friendly enough. Also the usefulness of the windowed comb metric is limited because we cannot communicate to downstream the level of combing and so the level of deinterlacing to be applied in the interlaced case. This could be added later as buffer metadata perhaps. Seeking? I need to look into that a bit more but wanted to get it more visibly into the wild. Perhaps some other things I am forgetting right now, but it's been looking pretty good for a little while now. Basic usage: some video input... ! fieldanalysis field-threshold=something frame-threshold=something ! deinterlace locking=auto ! ...
Created attachment 181214 [details] [review] Add fieldanalysis element The utility of this patch depends on some other patches that I will submit shortly.
Created attachment 181238 [details] [review] Steal a GstMiniObject flag for use as GST_BUFFER_FLAG_MEDIA4 This is necessary for explicit identification of buffers that are progressive within a telecine stream.
Created attachment 181239 [details] [review] gstvideo: Add GST_VIDEO_BUFFER_PROGRESSIVE flag Maps to GST_BUFFER_FLAG_MEDIA4. The purpose is to explicitly indicate whether a telecined buffer is progressive or not without having to make assumptions based on previous buffers.
https://bugzilla.gnome.org/show_bug.cgi?id=642691 is a pre-requisite for the following deinterlace attachment.
Created attachment 181248 [details] [review] deinterlace: Add support for deinterlacing using buffer caps/flags When not using the fieldanalysis element immediately upstream of deinterlace, behaviour should remain unchanged. fieldanalysis will set the caps and flags on the buffers such that they can be interpreted and acted upon to produce progressive output. There are two main modes of operation: - Passive pattern locking Passive pattern locking is a non-blocking, low-latency mode of operation that is suitable for close-to-live usage. Initially a telecine stream will be output as variable framerate with naïve timestamp adjustment. With each incoming buffer, an attempt is made to lock onto a pattern. When a lock is obtained, the src pad and output buffer caps will reflect the pattern and timestamps will be accurately interpolated between pattern repeats. This means that initially and at pattern transitions there will be short periods of inaccurate timestamping. - Active pattern locking Active pattern locking is a blocking, high-latency mode of operation that is targeted at use-cases where timestamp accuracy is paramount. Buffers will be queued until enough are present to make a lock. When locked, timestamps will be accurately interpolated between pattern repeats. Orphan fields can be dropped or deinterlaced. If no lock can be obtained, a single field might be pushed through to be deinterlaced. A third mode of operation, 'auto' chooses between passive and active locking modes depending on whether upstream is live.
I have tried to preserve the previous modes of operation with the default property values. If you wish to use pattern locking you need to specify the locking property and the appropriate value.
Just a note that arose from some discussion in the interlace pattern extension ticket. David said that interlace should be able to create a pattern on-the-fly according to the input and output frame rates when telecining. Such a pattern would probably not be lockable with the above deinterlace patch. Without pattern locking in the deinterlace patch, frames will be reconstructed but the timestamp adjustment is inaccurate. To do it accurately we at least need to have some ratio of input buffers to output buffers to be able to scale the input buffer duration to the output buffer duration and so fix up the timestamps. I originally did something hackish with the caps such that videorate could be used after deinterlace to fix the timestamps but it was ugly and so has been removed prior to submission. We could use a moving average of the number of output buffers per the last N input buffers and scale the timestamps/durations according to that perhaps...?
Review of attachment 181239 [details] [review]: Please push this patch
(In reply to comment #8) > Review of attachment 181239 [details] [review]: > > Please push this patch But please add something to the documentation that explains how telecined content can be distinguished from interlaced content and all that :)
Review of attachment 181214 [details] [review]: Looks good in general (of course I didn't review the details of the algorithms) but please don't use // comments and some other comments below. After fixing these feel free to push this to -bad :) ::: gst/fieldanalysis/gstfieldanalysis.c @@ +333,3 @@ + gst_element_add_pad (GST_ELEMENT (filter), filter->srcpad); + + filter->frames = g_queue_new (); You're leaking this. First of all the queue should probably be cleared on caps changes and when going back to READY and then it should be freed in GObject::finalize(). And flush the queue on FLUSH_START events and maybe NEWSEGMENT events with update=false. Note that the queue does not automatically unref the buffers if you clear it. @@ +550,3 @@ + GstCaps *caps; + + caps = gst_caps_copy (GST_PAD_CAPS (filter->srcpad)); Maybe add a fast-path here if the srcpad caps are not to be changed. Then you don't need to allocate new caps and do all the logic below but only need to set buffer flags. This code here sets new caps on every single buffer, which then requires expensive deep caps-equality checks downstream instead of just comparing the pointers. @@ +871,3 @@ + + comb_mask = (guint8 *) g_malloc (width); + block_scores = (guint *) g_malloc0 ((width / block_width) * sizeof (guint)); Maybe allocate this memory a single time in set_caps instead of allocating it for every single frame. Same goes for the other detect functions below
Review of attachment 181248 [details] [review]: In general looks good I guess. Some minor comments below but I didn't review every single line. If you're sure that all this works as expected feel free to push it after fixing the GAP stuff mentioned below ::: gst/deinterlace/gstdeinterlace.c @@ +3,3 @@ * Copyright (C) 2005 Martin Eikermann <meiker@upb.de> * Copyright (C) 2008-2010 Sebastian Dröge <slomo@collabora.co.uk> + * Copyright (C) 2010 Robert Swain <robert.swain@collabora.co.uk> We have 2011 :) The same for the fieldanalysis patch btw ;) @@ +548,3 @@ + * processing latency and accuracy of timestamp adjustment for telecine + * streams. + * Since markers please @@ +935,3 @@ + 0), "interlacing-method"); + /* 8 is strlen "telecine" */ + if (temp && strlen (temp) >= 8 && !strncmp (temp, "telecine", 8)) { You could use sizeof("telecine") here and why don't you just use g_str_equal() for example? :) @@ +1021,3 @@ + if (GST_BUFFER_FLAG_IS_SET (buffer, GST_BUFFER_FLAG_GAP)) { + /* gap buffers don't get added to the field history as they are essentially + * dropped */ Are they recreated later? They might be added by a videorate element for example and if you don't recreate them you'll create timestamp gaps. At least for interlaced content this should be handled like every other frame @@ +2134,3 @@ + /* 8 is strlen "telecine" */ + if (caps_field_temp && strlen (caps_field_temp) >= 8 + && !strncmp (caps_field_temp, "telecine", 8)) same as above, why not just g_str_equal()?
(In reply to comment #9) > (In reply to comment #8) > > Review of attachment 181239 [details] [review] [details]: > > > > Please push this patch > > But please add something to the documentation that explains how telecined > content can be distinguished from interlaced content and all that :) Bah, I remembered that you wanted this pushing, but forgot that you wanted more documentation. I'll add that and push it too.
Created attachment 182187 [details] [review] Add fieldanalysis element This updated version should fix all the issues from Sebastian's review of the previous patch.
Created attachment 182193 [details] [review] deinterlace: Add support for deinterlacing using buffer caps/flags Just the GAP buffers stuff left from Sebastian's review. I also converted all the interlacing method stuff to use an enum for readability.
What's missing here now? Can this be closed?
"Just the GAP buffers stuff left from Sebastian's review." for the deinterlace element. I will get to it when I have time.
Ah, would you mind to create another bug for this against gst-plugins-good and close this one?
Comment on attachment 182187 [details] [review] Add fieldanalysis element commit 14fb72014982c26a0847e9faeacac4fc25b738be Author: Robert Swain <robert.swain@collabora.co.uk> Date: Wed Sep 15 17:32:09 2010 +0200 fieldanalysis: Add fieldanalysis element This element analyses video buffers to identify if they are progressive, interlaced or telecined and outputs buffers with appropriate flags for a downstream element (which will be the deinterlace element, after some forthcoming modifications) to be able to output progressive frames and adjust timestamps resulting in a progressive stream.
Set to NEEDINFO as per comment #17
(In reply to comment #17) > Ah, would you mind to create another bug for this against gst-plugins-good and > close this one? No, that would make sense. I'll do it now.
New ticket created for deinterlace at: https://bugzilla.gnome.org/show_bug.cgi?id=643847