"Same as source" FPS Improvements

[jbrjake]

Back before the last release, I asked dynaflash to remove "Same as source" from the framerate menu in the MacGui, because it was so horribly broken.

Within, like, 15 minutes, someone pointed out that it did indeed work for detecting PAL vs. NTSC, and this person, with both PAL and NTSC discs, enjoyed the feature very much. So it got thrown back in.

Of course, the problems are enduring. HandBrake's FPS problems have even been highlighted for scorn by jwz himself. Despite being in the FAQ, every week someone comes along suffering from "Same as source"'s stupidity.

Let me explain in detail what's going on with Same as source. All it does is not specify a frame rate, so HandBrake sticks with what it decided to use when it scanned the disc. However, there's a little bit of code in decmpeg2.c (the interface with libmpeg2's mpeg2dec) that switches things up a bit:

Code: Select all

if( m->rate == 900900 )
{
    /* 29.97 fps. 3:2 pulldown might, or might not be
       used. I can't find a way to know, so we always
       output 23.976 */
    m->rate = 1126125;
}

Anotherwords, whenever the scanned fps is 29.97, it changes it to 23.976. Blindly. Naively.

My first idea was to apply some crude heuristics. Most 16x9 NTSC content is film speed, most 4x3 NTSC content is video speed. So instead of decimating all NTSC stuff to 23.976, only do it for widescreen.

That didn't work out too well, because of all the 4:3 NTSC TV-on-DVD boxed sets that are at 23.976.

So instead, I'm thinking of looking at the flag info we get about the frames from libmpeg2. The first step was patching libmpeg2 to see the flag REPEAT_FIRST_FIELD, which I ripped off of mencoder:

Code: Select all

diff -ur orig/header.c mpeg2dec/libmpeg2/header.c
--- orig/header.c	2003-12-22 12:24:02.000000000 +0100
+++ mpeg2dec/libmpeg2/header.c	2004-08-02 18:07:50.000000000 +0200
@@ -100,6 +100,9 @@
     mpeg2dec->decoder.convert = NULL;
     mpeg2dec->decoder.convert_id = NULL;
     mpeg2dec->picture = mpeg2dec->pictures;
+    memset(&mpeg2dec->fbuf_alloc[0].fbuf, 0, sizeof(mpeg2_fbuf_t));
+    memset(&mpeg2dec->fbuf_alloc[1].fbuf, 0, sizeof(mpeg2_fbuf_t));
+    memset(&mpeg2dec->fbuf_alloc[2].fbuf, 0, sizeof(mpeg2_fbuf_t));
     mpeg2dec->fbuf[0] = &mpeg2dec->fbuf_alloc[0].fbuf;
     mpeg2dec->fbuf[1] = &mpeg2dec->fbuf_alloc[1].fbuf;
     mpeg2dec->fbuf[2] = &mpeg2dec->fbuf_alloc[2].fbuf;
@@ -551,6 +554,7 @@
 	if (!(mpeg2dec->sequence.flags & SEQ_FLAG_PROGRESSIVE_SEQUENCE)) {
 	    picture->nb_fields = (buffer[3] & 2) ? 3 : 2;
 	    flags |= (buffer[3] & 128) ? PIC_FLAG_TOP_FIELD_FIRST : 0;
+	    flags |= (buffer[3] &   2) ? PIC_FLAG_REPEAT_FIRST_FIELD : 0;
 	} else
 	    picture->nb_fields = (buffer[3]&2) ? ((buffer[3]&128) ? 6 : 4) : 2;
 	break;
diff -ur orig/mpeg2.h mpeg2dec/include/mpeg2.h
--- orig/mpeg2.h	2003-12-22 13:13:35.000000000 +0100
+++ mpeg2dec/include/mpeg2.h	2004-02-18 13:50:13.000000000 +0100
@@ -82,6 +82,7 @@
 #define PIC_FLAG_COMPOSITE_DISPLAY 32
 #define PIC_FLAG_SKIP 64
 #define PIC_FLAG_TAGS 128
+#define PIC_FLAG_REPEAT_FIRST_FIELD 256
 #define PIC_MASK_COMPOSITE_DISPLAY 0xfffff000
 
 typedef struct mpeg2_picture_s {

To see what's going on, I added in some verbose logging to libhb/decmpeg2.c:

Code: Select all

 hb_log("MPEG 2 Info for PTS %lld", m->last_pts);
 if( m->info->display_picture->flags & PIC_FLAG_TOP_FIELD_FIRST )
     hb_log("MPEG2 Flag: Top field first");
 if( m->info->display_picture->flags & PIC_FLAG_PROGRESSIVE_FRAME )
     hb_log("MPEG2 Flag: Progressive");
 if( m->info->display_picture->flags & PIC_FLAG_COMPOSITE_DISPLAY )
     hb_log("MPEG2 Flag: Composite");
 if( m->info->display_picture->flags & PIC_FLAG_SKIP )
     hb_log("MPEG2 Flag: Skip!");
 if( m->info->display_picture->flags & PIC_FLAG_TAGS )
     hb_log("MPEG2 Flag: TAGS");
 if( m->info->display_picture->flags & PIC_FLAG_REPEAT_FIRST_FIELD )
     hb_log("MPEG2 Flag: REPEAT FIRST FIELD");

Now, when I scan discs, I can see stuff like this on a 23.976 film:

Scanning title 1 of 5...
[13:28:40] scan: preview 1
[13:28:40] MPEG 2 Info for PTS -1
[13:28:40] MPEG2 Flag: Top field first
[13:28:40] MPEG2 Flag: Progressive
[13:28:40] MPEG2 Flag: TAGS
[13:28:40] scan: preview 2
[13:28:40] MPEG 2 Info for PTS -1
[13:28:40] MPEG2 Flag: Progressive
[13:28:40] MPEG2 Flag: TAGS
[13:28:40] MPEG2 Flag: REPEAT FIRST FIELD
[13:28:40] scan: preview 3
[13:28:40] MPEG 2 Info for PTS -1
[13:28:40] MPEG2 Flag: Progressive
[13:28:40] MPEG2 Flag: TAGS

And this on a 29.97 video with hard telecining:

[13:36:43] scan: preview 1
[13:36:43] MPEG 2 Info for PTS -1
[13:36:43] MPEG2 Flag: Top field first
[13:36:43] MPEG2 Flag: TAGS
[13:36:43] scan: preview 2
[13:36:43] MPEG 2 Info for PTS -1
[13:36:43] MPEG2 Flag: Top field first
[13:36:43] MPEG2 Flag: TAGS
[13:36:43] scan: preview 3
[13:36:44] MPEG 2 Info for PTS -1
[13:36:44] MPEG2 Flag: Top field first
[13:36:44] MPEG2 Flag: TAGS

So. It seems to me that this could be an easy way of autodetecting NTSC frame rate. See the progressive and repeat flags in most of the preview scans? Assume the title's 23.976. Otherwise? 29.97.

Obviously, this won't solve anything for mixed content, and sure -- those preview scans will be wrong some of the time -- but it's a start.

I imagine we can extend this further, too. In my testing, running detelecine on truly interlaced content hasn't caused any problems. Might make sense to make that the default when scanning detects NTSC with no progressive or repeat first field flags, unless I'm missing something.

[awk]

I like the sound of this approach - it seems to be a positive improvement.

It clearly won't work for content which changes 'mid-stream' but I wonder whether that's really such a frequent occurence ? I can certainly see different titles on a single DVD having different rates - but mid stream ?

One example - the 'program content' of a TV -> DVD series such as Lost (Grey's Anatomy, 24) where the original footage is shot on film at 24fps and the DVD follows that. However the 'extra features' of behind the scenes or cast/crew interviews are shot using Video Cameras at 30i or similar, and those pieces are encoded at the matching rate. On those TV box-set DVD's these items are seperate titles, not a single title encompassing both pieces.

It seems like your patch will apply detection on a per title level ? In which case I'd argue that it catches the vast majority of mixed rate DVDs out there.

Andrew

P.S. Certainly the documentaries may include clips of content from the feature TV program. But usually that content is telecined to 30i prior to the editing of the documentary since editing mixed rate media on most editing systems (recent upgrades to FCP notwithstanding) is a royal pain (or simply not possible).

[rhester]

It clearly won't work for content which changes 'mid-stream' but I wonder whether that's really such a frequent occurence ? I can certainly see different titles on a single DVD having different rates - but mid stream ?

Very, very often. Every HBO DVD ever released uses this strategy (especially with mixed live video and CG), and a great many companies use it with TV shows where the intro and credits are done as interlaced and the show proper is NTSC film.

It's a lot more common than you think, but does seem mostly limited to TV->DVD transfers.

Rodney

[jbrjake]

It clearly won't work for content which changes 'mid-stream' but I wonder whether that's really such a frequent occurence ?

Like rhester says, it is more common than you'd think. But no, it wouldn't work for those. I just want something better than "All NTSC is progressive." like it is now ;>

It seems like your patch will apply detection on a per title level ? In which case I'd argue that it catches the vast majority of mixed rate DVDs out there.

Yep, that's exactly what this does. With what I'm working on now, I've got HandBrake determining if a title is mostly progressive or not during the scan. So, for example, it can figure out that the main title on a DVD is progressive but the extras are all interlaced (or hard telecined -- my code can't tell the difference between it and interlacing).

Every HBO DVD ever released uses this strategy (especially with mixed live video and CG), and a great many companies use it with TV shows where the intro and credits are done as interlaced and the show proper is NTSC film.

Since we're already looking at 10 frames spaced evenly throughout the title, during the initial scan, I'm hoping it will be possible to do something like what eddyg has going on with his subtitle scan. If, say, 6 out of 10 of the preview frames register as progressive, then mark the whole title as progressive.

Working example:

Scanning title 1 of 5...
[20:24:29] scan: preview 1
[20:24:29] scan: AC3, rate=48000Hz, bitrate=192000
[20:24:29] scan: AC3, rate=48000Hz, bitrate=192000
[20:24:29] scan: AC3, rate=48000Hz, bitrate=192000
[20:24:29] scan: AC3, rate=48000Hz, bitrate=192000
[20:24:29] scan: checking for DCA syncinfo
[20:24:29] scan: DCA, rate=48000Hz, bitrate=768000
[20:24:29] scan: AC3, rate=48000Hz, bitrate=448000
[20:24:29] Detecting NTSC Progressive Frame
[20:24:29] scan: preview 2
[20:24:29] Detecting NTSC Progressive Frame
[20:24:29] scan: preview 3
[20:24:30] Detecting NTSC Progressive Frame
[20:24:30] scan: preview 4
[20:24:30] Detecting NTSC Progressive Frame
[20:24:30] scan: preview 5
[20:24:30] Detecting NTSC Progressive Frame
[20:24:30] scan: preview 6
[20:24:30] Detecting NTSC Progressive Frame
[20:24:30] Title's mostly progressive NTSC, setting fps to 23.976

Again, it won't be any help with heavily mixed content. I wouldn't expect it to work, say, on Babylon 5 or early seasons of Angel where any random effects shot might screw the cadence.

[jbrjake]

So things are a little clearer, here's my current diff:

Code: Select all

Index: libhb/decmpeg2.c
===================================================================
--- libhb/decmpeg2.c	(revision 783)
+++ libhb/decmpeg2.c	(working copy)
@@ -81,13 +81,6 @@
                 m->height = m->info->sequence->height;
                 m->rate   = m->info->sequence->frame_period;
                 
-                if( m->rate == 900900 )
-                {
-                    /* 29.97 fps. 3:2 pulldown might, or might not be
-                       used. I can't find a way to know, so we always
-                       output 23.976 */
-                    m->rate = 1126125;
-                }
             }
             if ( m->aspect_ratio <= 0 )
             {
@@ -204,6 +197,11 @@
 {
     *width  = m->width;
     *height = m->height;
+    if( (m->info->display_picture->flags & PIC_FLAG_PROGRESSIVE_FRAME) && (m->height == 480) )
+      {
+          hb_log("Detecting NTSC Progressive Frame");
+          m->rate = 1126125;
+      }
     *rate   = m->rate;
     *aspect_ratio = m->aspect_ratio;
 }
Index: libhb/scan.c
===================================================================
--- libhb/scan.c	(revision 783)
+++ libhb/scan.c	(working copy)
@@ -290,6 +290,8 @@
     hb_buffer_t   * buf_ps, * buf_es, * buf_raw;
     hb_list_t     * list_es, * list_raw;
     hb_libmpeg2_t * mpeg2;
+    int progressive_count;
+    progressive_count = 0;
     
     buf_ps   = hb_buffer_init( HB_DVD_READ_BUFFER_SIZE );
     list_es  = hb_list_init();
@@ -377,20 +379,30 @@
             goto error;
         }
 
-        if( !i )
+        /* Get size and rate infos */
+        title->rate = 27000000;
+        int ar;
+        hb_libmpeg2_info( mpeg2, &title->width, &title->height,
+                          &title->rate_base, &ar );
+        
+        if (title->rate_base == 1126125)
         {
-            /* Get size and rate infos */
-            title->rate = 27000000;
-            int ar;
-            hb_libmpeg2_info( mpeg2, &title->width, &title->height,
-                              &title->rate_base, &ar );
-            // The aspect ratio may have already been set by parsing the VOB/IFO details on a DVD, however
-            // if we're working with program/transport streams that data needs to come from within the stream.
-            if (title->aspect <= 0)
-              title->aspect = ar;
-            title->crop[0] = title->crop[1] = title->height / 2;
-            title->crop[2] = title->crop[3] = title->width / 2;
+            progressive_count++;
+            title->rate_base = 900900;            
         }
+        
+        if (progressive_count == 6)
+        {
+            hb_log("Title's mostly progressive NTSC, setting fps to 23.976");
+            title->rate_base = 1126125;
+        }
+        
+        // The aspect ratio may have already been set by parsing the VOB/IFO details on a DVD, however
+        // if we're working with program/transport streams that data needs to come from within the stream.
+        if (title->aspect <= 0)
+          title->aspect = ar;
+        title->crop[0] = title->crop[1] = title->height / 2;
+        title->crop[2] = title->crop[3] = title->width / 2;
 
         hb_libmpeg2_close( &mpeg2 );
 

[jbrjake]

Back with something a little more ready-to-check-in. Dynaflash says I should go ahead and commit. Any dissenters?

Fixed a few minor bugs (like not cumulatively adding the cropping from all 10 preview frames -- oops) and made a slight change so that cropping info comes from 30% into the picture instead of right at the beginning.

Oh yeah, and comments. Lots of comments.

Code: Select all

Index: libhb/decmpeg2.c
===================================================================
--- libhb/decmpeg2.c	(revision 783)
+++ libhb/decmpeg2.c	(working copy)
@@ -81,13 +81,6 @@
                 m->height = m->info->sequence->height;
                 m->rate   = m->info->sequence->frame_period;
                 
-                if( m->rate == 900900 )
-                {
-                    /* 29.97 fps. 3:2 pulldown might, or might not be
-                       used. I can't find a way to know, so we always
-                       output 23.976 */
-                    m->rate = 1126125;
-                }
             }
             if ( m->aspect_ratio <= 0 )
             {
@@ -204,6 +197,16 @@
 {
     *width  = m->width;
     *height = m->height;
+    if( (m->info->display_picture->flags & PIC_FLAG_PROGRESSIVE_FRAME) && (m->height == 480) )
+      {
+          /* The frame is progressive and it's NTSC DVD height, so change its FPS to 23.976.
+             This might not be correct for the title. It's really just for scan.c's benefit.
+             Scan.c will reset the fps to 29.97, until a simple majority of the preview
+             frames report at 23.976.
+          */
+          hb_log("Detecting NTSC Progressive Frame");
+          m->rate = 1126125;
+      }
     *rate   = m->rate;
     *aspect_ratio = m->aspect_ratio;
 }
Index: libhb/scan.c
===================================================================
--- libhb/scan.c	(revision 783)
+++ libhb/scan.c	(working copy)
@@ -290,6 +290,7 @@
     hb_buffer_t   * buf_ps, * buf_es, * buf_raw;
     hb_list_t     * list_es, * list_raw;
     hb_libmpeg2_t * mpeg2;
+    int progressive_count = 0;

     buf_ps   = hb_buffer_init( HB_DVD_READ_BUFFER_SIZE );
     list_es  = hb_list_init();
@@ -377,13 +378,40 @@
             goto error;
         }

-        if( !i )
+        /* Get size and rate infos */
+        title->rate = 27000000;
+        int ar;
+        hb_libmpeg2_info( mpeg2, &title->width, &title->height,
+                          &title->rate_base, &ar );
+        
+        if (title->rate_base == 1126125)
         {
-            /* Get size and rate infos */
-            title->rate = 27000000;
-            int ar;
-            hb_libmpeg2_info( mpeg2, &title->width, &title->height,
-                              &title->rate_base, &ar );
+            /* Frame FPS is 23.976 (meaning it's progressive), so
+               start keeping track of how many are reporting at
+               that speed. When enough show up that way, we want
+               to make that the overall title FPS.
+            */
+            progressive_count++;
+
+            if (progressive_count < 6)
+                /* Not enough frames are reporting as progressive,
+                   which means we should be conservative and use
+                   29.97 as the title's FPS for now.
+                */ 
+                title->rate_base = 900900;            
+            else
+            {
+                /* A majority of the scan frames are progressive. Make that
+                    the title's FPS, and announce it once to the log.
+                */
+                if (progressive_count == 6)
+                    hb_log("Title's mostly progressive NTSC, setting fps to 23.976");
+                title->rate_base = 1126125;                
+            }
+        }
+                
+        if( i == 2) // Use the third frame's info, so as to skip opening logos
+        {
             // The aspect ratio may have already been set by parsing the VOB/IFO details on a DVD, however
             // if we're working with program/transport streams that data needs to come from within the stream.
             if (title->aspect <= 0)

EDIT: Oh yeah...and rhester? What HBO DVDs in particular do that nasty switcharoo? This scan stuff is working great for me on Carnivale, and I think the only other HBO discs I have are Mr. Show, which are interlaced.

[dynaflash]

Dynaflash says I should go ahead and commit.

I say alot of things, I didnt think anyone ever listened.

[cvk_b]

Maybe instead of "Same As Source" it should be called "Best Guess" or "Auto Detected" or "Per Scan". No matter what it's called, it should be the default. I hope this makes this release

[gbooker]

Since we're already looking at 10 frames spaced evenly throughout the title, during the initial scan, I'm hoping it will be possible to do something like what eddyg has going on with his subtitle scan. If, say, 6 out of 10 of the preview frames register as progressive, then mark the whole title as progressive.

I would argue that this is not sufficient. I have some DVDs with interlaced content, and in some cases, the 10 preview frames do not show any sign of interlacing. Add on to examining the frame preceding and the frame following each of these 10 frames, and I think you have a robust detection mechanism.

Again, it won't be any help with heavily mixed content. I wouldn't expect it to work, say, on Babylon 5 or early seasons of Angel where any random effects shot might screw the cadence.

An idea for down the road:
Both MKV and MP4 can do variable frame rate video. With each frame, you give it a presentation time stamp and it knows when to display that frame. I don't know about OGM; I just don't know the container at all. AVI can't do this, but then I would argue that AVI is a format that needs to die. Anyway, using this method for MKV/MP4 would be the ultimate solution in the end.

[rhester]

Oh yeah...and rhester? What HBO DVDs in particular do that nasty switcharoo? This scan stuff is working great for me on Carnivale, and I think the only other HBO discs I have are Mr. Show, which are interlaced.

From the Earth to the Moon and Band of Brothers are both great examples in my collection.

Rodney

[rhester]

An idea for down the road:
Both MKV and MP4 can do variable frame rate video. With each frame, you give it a presentation time stamp and it knows when to display that frame. I don't know about OGM; I just don't know the container at all. AVI can't do this, but then I would argue that AVI is a format that needs to die. Anyway, using this method for MKV/MP4 would be the ultimate solution in the end.

This is by far the optimal solution and one I have looked into a bit - unfortunately, implementing VFR seems to be rather challenging thus far. If you have thoughts on a technical approach, please share.

Rodney

[awk]

Very, very often. Every HBO DVD ever released uses this strategy (especially with mixed live video and CG), and a great many companies use it with TV shows where the intro and credits are done as interlaced and the show proper is NTSC film.

It's a lot more common than you think, but does seem mostly limited to TV->DVD transfers.

Rodney

That's disappointing - I'll have too look into the background of the production of these titles a little and see why things fall out this way (it's academic, nothing's going to change, but it actually relates to my 'day job' .

Someone mentioned 'Band Of Brothers' as one (funnily enough I met with the editor for that series about 3 weeks ago . Any other titles that immediately spring to mind ?

[rhester]

That's disappointing - I'll have too look into the background of the production of these titles a little and see why things fall out this way (it's academic, nothing's going to change, but it actually relates to my 'day job' .

I think it's more for reuse - you encode the intro/standard credits once and just graft it onto the beginning and the end of each episode. As for why the "bookends" are interlaced video (which makes sense, because there truly is 30fps of data there) but the episodes are hard-telecined film (did they intentionally drop data to get from 30fps to 24fps?), I have no clue, but this is remarkably common practice.

Someone mentioned 'Band Of Brothers' as one (funnily enough I met with the editor for that series about 3 weeks ago . Any other titles that immediately spring to mind ?

From my own experience...

Mixed film (live action) and video (CG):

From the Earth to the Moon
Band of Brothers

Intro/credits as video, episode as film:

Sex and the City

I don't own a copy, but I'm reasonably certain Sopranos would also fall into this.

NBC titles, like Friends, appear to be hard-telecined film from beginning to end.

Rodney

[jbrjake]

I would argue that this is not sufficient. I have some DVDs with interlaced content, and in some cases, the 10 preview frames do not show any sign of interlacing. Add on to examining the frame preceding and the frame following each of these 10 frames, and I think you have a robust detection mechanism.

Just to be clear, you're saying you have interlaced DVDs where all 10 preview frames have a progressive MPEG-2 picture flag?

Or just that the preview frames don't visibly show interlacing combing in HandBrake's preview window?

Examining surrounding frames is kinda difficult with the way scanning is set up. To be duration-agnostic, the scanner just sends percentages to the DVD reader -- "seek to 10%", "seek to 20%" etc. Then the reader converts that to a block count for the specific title. Getting the exact preceding and succeeding frames might very well be beyond my capabilities. I'm playing around with it now, but I don't see an easy way to choose an exact frame to seek to, without adding some new functions to dvd.c. Not sure I want to pursue that at this point in the release cycle. All I want is something better than the current system...I know it won't be perfect.

You're right that VFR is the optimal way of handling this, but my goal at the moment is just to make "Same as source" not default to 23.976 for interlaced/hard telecined*/mixed content.

* For now HandBrake's ivtc filter can't drop duped frames, and needs 29.97 as the frame rate for hard telecined material.

[gbooker]

I would argue that this is not sufficient. I have some DVDs with interlaced content, and in some cases, the 10 preview frames do not show any sign of interlacing. Add on to examining the frame preceding and the frame following each of these 10 frames, and I think you have a robust detection mechanism.

Just to be clear, you're saying you have interlaced DVDs where all 10 preview frames have a progressive MPEG-2 picture flag?

Or just that the preview frames don't visibly show interlacing combing in HandBrake's preview window?

I was meaning nothing visible. Although, it could very well have that flag set. The DVDs have mixed film/CGI content, and the CGI content appears to be 30fps where as the film is 24. For reference, I was using the Babylon 5 DVDs.

Examining surrounding frames is kinda difficult with the way scanning is set up. To be duration-agnostic, the scanner just sends percentages to the DVD reader -- "seek to 10%", "seek to 20%" etc. Then the reader converts that to a block count for the specific title. Getting the exact preceding and succeeding frames might very well be beyond my capabilities. I'm playing around with it now, but I don't see an easy way to choose an exact frame to seek to, without adding some new functions to dvd.c. Not sure I want to pursue that at this point in the release cycle. All I want is something better than the current system...I know it won't be perfect.

Drat, I was hoping it was frame numbers, but if you are just looking at flags, then I don't think there is any need to do this (I am not sure exactly how the flags are set. Are they never set on any frames part of a 3:2 pulldown or are they set on the frames which are unmodified?).

You're right that VFR is the optimal way of handling this, but my goal at the moment is just to make "Same as source" not default to 23.976 for interlaced/hard telecined*/mixed content.

* For now HandBrake's ivtc filter can't drop duped frames, and needs 29.97 as the frame rate for hard telecined material.

Yes, which is why I said "for down the road". Just stating where I hope things go in the future, but understand it won't be there immediately.

[jbrjake]

I was meaning nothing visible. Although, it could very well have that flag set. The DVDs have mixed film/CGI content, and the CGI content appears to be 30fps where as the film is 24. For reference, I was using the Babylon 5 DVDs.

Well, I can pretty much guarantee you that what I'm doing right now will fail on B5 =( I've been testing on Buffy and Angel, which are almost as bad of offenders when it comes to unpredictable mixed content.

With those two, at least, most of the progressive sequences are marked as progressive and the hard telecined/interlaced bits aren't. Because scanning is looking at a frame from every 10% of the title, it doesn't get tricked up by telecined "bookends" for "Previously..."s and opening credits, if the majority of the title is marked progressive. Then with some motion-adaptive interlacing, the telecined bits are somewhat acceptable (albeit jerky, of course).

The problem is when the progressive parts screw up because of a missing repeat/TFF flag and break the cadence. That's why I doubt B5 will work out well. It happens a lot in that series, like when they re-use establishing panning shots of the Zocalo. I've also seen it in Angel+Buffy and some episodes of Brisco County. It's a pretty prevalent problem with TV-on-DVD from the 90s. Luckily, it usually only lasts a few hundred frames.

But then, HandBrake isn't handling that stuff now anyway.

(I am not sure exactly how the flags are set. Are they never set on any frames part of a 3:2 pulldown or are they set on the frames which are unmodified?).

It seems to me that the progressive flag is set on all (usually, see below) frames in progressive content. When I peek at the picture flags while encoding, it seems to show up every frame in progressive sequences, but not in telecined/interlaced sequences like the bookends rhester's described. On a Criterion disc where the main title should definitely play at 23.976, I get progressive=0 for the Criterion logo at the beginning, and then it goes progressive=1 as soon as the actual film starts, and stays that way.

I've been doing some research on this today, and found a great article on the subject:
http://www.hometheaterhifi.com/volume_7 ... -2000.html

It seems that the biggest problem is going to be that some DVDs, like Titanic and the first Austin Powers, only set the progressive flag on every other frame. So my "if 6 out of 10 are progressive" rule might be too conservative for those...I'm considering doubling the number of scans, to 20, to compensate.

The worst scenario will be that sometimes interlaced material gets progressively flagged for no good reason. But that article assures me that this is "rare" and the examples they give are things like menu intros and the occasional DVD extra featurette.

My hope is that these changes will at least make HandBrake detect progressive in the same cases where MPEG Streamclip would.

Oh yeah, I've also been spending a lot of time reading this:
http://209.85.165.104/search?q=cache:Lq ... q_v38.html
...trying to figure out if there's a way to detect hard telecining so we can auto-enable ivtc.

[jbrjake]

Well, I've got some very rudimentary frame rate detection working. Hopefully, a version or three down the line, someone might be able to get something out of this stuff when beginning variable frame rate work.

By keeping track of the previous 2 frames' picture structure flags (obviously this should be extended to track more frames), I'm able to note when the stream goes from TOP_BOTTOM or BOTTOM_TOP to progressive combos.

So, for example, when encoding the first episode of Angel, I see this output:

Encoding: task 1 of 1, 0.00 %
DVD: Beginning of Cell
[15:08:52] 20378: Progressive -> Interlaced
[15:08:53] sync: first pts is 17375
Encoding: task 1 of 1, 0.76 % (66.94 fps, avg 67.68 fps, ETA 00h15m39s)
[15:09:00] 1897253: Interlaced -> Progressive
Encoding: task 1 of 1, 5.37 % (65.50 fps, avg 66.56 fps, ETA 00h15m09s)
[15:09:44] 12964808: Progressive -> Interlaced
Encoding: task 1 of 1, 6.47 % (65.94 fps, avg 66.14 fps, ETA 00h15m05s)
[15:09:55] 15572915: Interlaced -> Progressive
Encoding: task 1 of 1, 6.53 % (67.27 fps, avg 66.23 fps, ETA 00h15m03s)
[15:09:56] 15726067: Progressive -> Interlaced
Encoding: task 1 of 1, 7.68 % (66.43 fps, avg 66.04 fps, ETA 00h14m54s)
DVD: End of Cell

I'm debating whether or not I should check this in. I figure it could be useful for support.

"Help, my video is jerky!!!"
"Okay, show us the activity log..."
"Here ya go."
"Ooohhhh, I see a bunch of cadence changes. Sorry, this is heavily mixed content and HandBrake can't encode it properly."

Should be easy enough to extend it to see when the 3:2 pattern for soft telecining changes or is interrupted or when interlaced content switches up top bottom and bottom first.

Code: Select all

Index: libhb/decmpeg2.c
===================================================================
--- libhb/decmpeg2.c	(revision 783)
+++ libhb/decmpeg2.c	(working copy)
@@ -8,6 +8,17 @@
 
 #include "mpeg2dec/mpeg2.h"
 
+/* Cadence tracking */
+#define TB 8
+#define BT 16
+#define BT_PROG 32
+#define BTB_PROG 64
+#define TB_PROG 128
+#define TBT_PROG 256
+int cadence = 0;
+int cadence_last = 0;
+int cadence_laster = 0;
+
 /**********************************************************************
  * hb_libmpeg2_t
  **********************************************************************
@@ -80,14 +91,6 @@
                 m->width  = m->info->sequence->width;
                 m->height = m->info->sequence->height;
                 m->rate   = m->info->sequence->frame_period;
-                
-                if( m->rate == 900900 )
-                {
-                    /* 29.97 fps. 3:2 pulldown might, or might not be
-                       used. I can't find a way to know, so we always
-                       output 23.976 */
-                    m->rate = 1126125;
-                }
             }
             if ( m->aspect_ratio <= 0 )
             {
@@ -180,6 +183,91 @@
                 }
                 m->last_pts = buf->start;
 
+/*  Uncomment this block to see frame-by-frame picture flags, as the video encodes.
+               hb_log("***** MPEG 2 Info for PTS %lld *****", m->last_pts);
+                if( m->info->display_picture->flags & PIC_FLAG_TOP_FIELD_FIRST )
+                    hb_log("MPEG2 Flag: Top field first");
+                if( m->info->display_picture->flags & PIC_FLAG_PROGRESSIVE_FRAME )
+                    hb_log("MPEG2 Flag: Progressive");
+                if( m->info->display_picture->flags & PIC_FLAG_COMPOSITE_DISPLAY )
+                    hb_log("MPEG2 Flag: Composite");
+                if( m->info->display_picture->flags & PIC_FLAG_SKIP )
+                    hb_log("MPEG2 Flag: Skip!");
+                if( m->info->display_picture->flags & PIC_FLAG_TAGS )
+                    hb_log("MPEG2 Flag: TAGS");
+                if( m->info->display_picture->flags & PIC_FLAG_REPEAT_FIRST_FIELD )
+                    hb_log("MPEG2 Flag: Repeat first field");
+                if( m->info->display_picture->flags & PIC_MASK_COMPOSITE_DISPLAY )
+                    hb_log("MPEG2 Flag: Composite mask");
+                hb_log("fields: %d", m->info->display_picture->nb_fields);
+*/              
+                /* Rotate the cadence tracking. */
+                cadence_laster = cadence_last;
+                cadence_last = cadence;
+                
+                if ( !(m->info->display_picture->flags & PIC_FLAG_PROGRESSIVE_FRAME) && !(m->info->display_picture->flags & PIC_FLAG_TOP_FIELD_FIRST) )
+                {
+                    /* Not progressive, not top first...
+                       That means it's probably bottom
+                       first, 2 fields displayed.
+                    */
+                    //hb_log("MPEG2 Flag: Bottom field first, 2 fields displayed.");
+                    cadence = BT;
+                } 
+                else if ( !(m->info->display_picture->flags & PIC_FLAG_PROGRESSIVE_FRAME) && (m->info->display_picture->flags & PIC_FLAG_TOP_FIELD_FIRST) )
+                {
+                    /* Not progressive, top is first,
+                       Two fields displayed.
+                    */
+                    //hb_log("MPEG2 Flag: Top field first, 2 fields displayed.");
+                    cadence = TB;
+                } 
+                else if ( (m->info->display_picture->flags & PIC_FLAG_PROGRESSIVE_FRAME) && !(m->info->display_picture->flags & PIC_FLAG_TOP_FIELD_FIRST) && !( m->info->display_picture->flags & PIC_FLAG_REPEAT_FIRST_FIELD )  )
+                {
+                    /* Progressive, but noting else.
+                       That means Bottom first,
+                       2 fields displayed.
+                    */
+                    //hb_log("MPEG2 Flag: Progressive. Bottom field first, 2 fields displayed.");
+                    cadence = BT_PROG;
+                } 
+                else if ( (m->info->display_picture->flags & PIC_FLAG_PROGRESSIVE_FRAME) && !(m->info->display_picture->flags & PIC_FLAG_TOP_FIELD_FIRST) && ( m->info->display_picture->flags & PIC_FLAG_REPEAT_FIRST_FIELD )  )
+                {
+                    /* Progressive, and repeat. .
+                       That means Bottom first,
+                       3 fields displayed.
+                    */
+                    //hb_log("MPEG2 Flag: Progressive repeat. Bottom field first, 3 fields displayed.");
+                    cadence = BTB_PROG;
+                } 
+                else if ( (m->info->display_picture->flags & PIC_FLAG_PROGRESSIVE_FRAME) && (m->info->display_picture->flags & PIC_FLAG_TOP_FIELD_FIRST) && !( m->info->display_picture->flags & PIC_FLAG_REPEAT_FIRST_FIELD )  )
+                {
+                    /* Progressive, top first.
+                       That means top first,
+                       2 fields displayed.
+                    */
+                    //hb_log("MPEG2 Flag: Progressive. Top field first, 2 fields displayed.");
+                    cadence = TB_PROG;
+                } 
+                else if ( (m->info->display_picture->flags & PIC_FLAG_PROGRESSIVE_FRAME) && (m->info->display_picture->flags & PIC_FLAG_TOP_FIELD_FIRST) && ( m->info->display_picture->flags & PIC_FLAG_REPEAT_FIRST_FIELD )  )
+                {
+                    /* Progressive, top, repeat.
+                       That means top first,
+                       3 fields displayed.
+                    */
+                    //hb_log("MPEG2 Flag: Progressive repeat. Top field first, 3 fields displayed.");
+                    cadence = TBT_PROG;
+                } 
+                                                
+                if ( (cadence_laster <= TB) && (cadence_last <= TB) && (cadence > TB) )
+                {
+                    hb_log("%lld: Interlaced -> Progressive", buf->start);
+                }
+                if ( (cadence_laster > TB) && (cadence_last <= TB) && (cadence <= TB) && (cadence) )
+                {
+                    hb_log("%lld: Progressive -> Interlaced", buf->start);
+                }
+
                 /* Store picture flags for later use by filters */
                 buf->flags = m->info->display_picture->flags;
                 
@@ -204,6 +292,16 @@
 {
     *width  = m->width;
     *height = m->height;
+    if( (m->info->display_picture->flags & PIC_FLAG_PROGRESSIVE_FRAME) && (m->height == 480) )
+      {
+          /* The frame is progressive and it's NTSC DVD height, so change its FPS to 23.976.
+             This might not be correct for the title. It's really just for scan.c's benefit.
+             Scan.c will reset the fps to 29.97, until a simple majority of the preview
+             frames report at 23.976.
+          */
+          hb_log("Detecting NTSC Progressive Frame");
+          m->rate = 1126125;
+      }
     *rate   = m->rate;
     *aspect_ratio = m->aspect_ratio;
 }

[eddyg]

Hi Jon,

Good work.

I was thinking that for 2-pass we could easily use your cadence checking code to find out what % of the title was interlaced and what % was progressive. And then select the options as appropriate for the second pass.

Obviously this will throw out the 1st pass bitrate calculations a tad if we change. But probably not that much.

The other idea was to integrate the cadence tracking with the subtitle scan for a quick pass through the title with no video encoding, reading and decoding only. Collect our info, and then automatically select the options based on that. I have yet to write that feature, but it is easy on the back of the 2-pass work.

Cheers, Ed.

[jbrjake]

Obviously this will throw out the 1st pass bitrate calculations a tad if we change. But probably not that much.

I have a feeling x264 will throw a hissy fit even if the changes are minor ;>

The other idea was to integrate the cadence tracking with the subtitle scan for a quick pass through the title with no video encoding, reading and decoding only. Collect our info, and then automatically select the options based on that. I have yet to write that feature, but it is easy on the back of the 2-pass work.

This sounds really good to me. I was thinking about doing the detection on the fly if we ever find a way to do VFR (by examining the raw fifo list for these flags) but doing it first would be a lot better. I wonder if we could also detect the audio sync issues in this pre-scan so as to compensate during the real encode?

I believe Windows folk do these pre-scans with DGIndex: http://www.animemusicvideos.org/guides/ ... etb2a.html

[eddyg]

I wonder if we could also detect the audio sync issues in this pre-scan so as to compensate during the real encode?l

My prototype doesn't decode the audio, just subtitles and video. But it wouldn't be hard to enable audio decoding.

We'd have to let everything get to sync before ditching it. But that's not hard.

The problem comes from what we do with this info..

I think we'd be better off pursuing a better sync methodology which allows lossless syncing from both directions video -> audio as well as audio -> video.

As far as the pre-scan is concerned, I'm not sure how to integrate that with the GUI. It takes a while to scan, and is a separate job. So I'd see it working by having a "auto-select" option, which would then grey out the subtitle and interlace options all together. Then when we schedule the jobs we have the scan pass=1 and the encode as pass=2.

Makes 2-pass harder though - for the reasons that you mentioned if we turn interlace on/off across passes.

Cheers, Ed.

[jbrjake]

I cleaned it up a little. This will tell you when the flags start and stop being progressive during an encode, sets the FPS to 23.976 if >5 preview frames are marked progressive, and grabs cropping info from the 3rd preview instead of the 1st..

Code: Select all

Index: libhb/decmpeg2.c
===================================================================
--- libhb/decmpeg2.c	(revision 783)
+++ libhb/decmpeg2.c	(working copy)
@@ -8,6 +8,23 @@
 
 #include "mpeg2dec/mpeg2.h"
 
+/* Cadence tracking */
+#define TOP_FIRST PIC_FLAG_TOP_FIELD_FIRST
+#define PROGRESSIVE PIC_FLAG_PROGRESSIVE_FRAME 
+#define COMPOSITE PIC_FLAG_COMPOSITE_DISPLAY
+#define SKIP PIC_FLAG_SKIP
+#define TAGS PIC_FLAG_TAGS
+#define REPEAT_FIRST PIC_FLAG_REPEAT_FIRST_FIELD
+#define COMPOSITE_MASK PIC_MASK_COMPOSITE_DISPLAY
+#define TB 8
+#define BT 16
+#define BT_PROG 32
+#define BTB_PROG 64
+#define TB_PROG 128
+#define TBT_PROG 256
+int cadence[6];
+int flag = 0;
+
 /**********************************************************************
  * hb_libmpeg2_t
  **********************************************************************
@@ -80,14 +97,6 @@
                 m->width  = m->info->sequence->width;
                 m->height = m->info->sequence->height;
                 m->rate   = m->info->sequence->frame_period;
-                
-                if( m->rate == 900900 )
-                {
-                    /* 29.97 fps. 3:2 pulldown might, or might not be
-                       used. I can't find a way to know, so we always
-                       output 23.976 */
-                    m->rate = 1126125;
-                }
             }
             if ( m->aspect_ratio <= 0 )
             {
@@ -180,6 +189,92 @@
                 }
                 m->last_pts = buf->start;
 
+                flag = m->info->display_picture->flags;
+                
+/*  Uncomment this block to see frame-by-frame picture flags, as the video encodes.
+               hb_log("***** MPEG 2 Picture Info for PTS %lld *****", buf->start);
+                if( flag & TOP_FIRST )
+                    hb_log("MPEG2 Flag: Top field first");
+                if( flag & PROGRESSIVE )
+                    hb_log("MPEG2 Flag: Progressive");
+                if( flag & COMPOSITE )
+                    hb_log("MPEG2 Flag: Composite");
+                if( flag & SKIP )
+                    hb_log("MPEG2 Flag: Skip!");
+                if( flag & TAGS )
+                    hb_log("MPEG2 Flag: TAGS");
+                if(flag & REPEAT_FIRST )
+                    hb_log("MPEG2 Flag: Repeat first field");
+                if( flag & COMPOSITE_MASK )
+                    hb_log("MPEG2 Flag: Composite mask");
+                hb_log("fields: %d", m->info->display_picture->nb_fields);
+*/              
+                /*  Rotate the cadence tracking. */
+                cadence[5] = cadence[4];
+                cadence[4] = cadence[3];
+                cadence[3] = cadence[2];
+                cadence[2] = cadence[1];
+                cadence[1] = cadence[0];
+                
+                if ( !(flag & PROGRESSIVE) && !(flag & TOP_FIRST) )
+                {
+                    /* Not progressive, not top first...
+                       That means it's probably bottom
+                       first, 2 fields displayed.
+                    */
+                    //hb_log("MPEG2 Flag: Bottom field first, 2 fields displayed.");
+                    cadence[0] = BT;
+                } 
+                else if ( !(flag & PROGRESSIVE) && (flag & TOP_FIRST) )
+                {
+                    /* Not progressive, top is first,
+                       Two fields displayed.
+                    */
+                    //hb_log("MPEG2 Flag: Top field first, 2 fields displayed.");
+                    cadence[0] = TB;
+                } 
+                else if ( (flag & PROGRESSIVE) && !(flag & TOP_FIRST) && !( flag & REPEAT_FIRST )  )
+                {
+                    /* Progressive, but noting else.
+                       That means Bottom first,
+                       2 fields displayed.
+                    */
+                    //hb_log("MPEG2 Flag: Progressive. Bottom field first, 2 fields displayed.");
+                    cadence[0] = BT_PROG;
+                } 
+                else if ( (flag & PROGRESSIVE) && !(flag & TOP_FIRST) && ( flag & REPEAT_FIRST )  )
+                {
+                    /* Progressive, and repeat. .
+                       That means Bottom first,
+                       3 fields displayed.
+                    */
+                    //hb_log("MPEG2 Flag: Progressive repeat. Bottom field first, 3 fields displayed.");
+                    cadence[0] = BTB_PROG;
+                } 
+                else if ( (flag & PROGRESSIVE) && (flag & TOP_FIRST) && !( flag & REPEAT_FIRST )  )
+                {
+                    /* Progressive, top first.
+                       That means top first,
+                       2 fields displayed.
+                    */
+                    //hb_log("MPEG2 Flag: Progressive. Top field first, 2 fields displayed.");
+                    cadence[0] = TB_PROG;
+                } 
+                else if ( (flag & PROGRESSIVE) && (flag & TOP_FIRST) && ( flag & REPEAT_FIRST )  )
+                {
+                    /* Progressive, top, repeat.
+                       That means top first,
+                       3 fields displayed.
+                    */
+                    //hb_log("MPEG2 Flag: Progressive repeat. Top field first, 3 fields displayed.");
+                    cadence[0] = TBT_PROG;
+                } 
+                                                
+                if ( (cadence[2] <= TB) && (cadence[1] <= TB) && (cadence[0] > TB) && (cadence[0]) && cadence[1])
+                    hb_log("PTS %lld: Interlaced -> Progressive", buf->start);
+                if ( (cadence[2] > TB) && (cadence[1] <= TB) && (cadence[0] <= TB) && (cadence[0]) && cadence[1])
+                    hb_log("PTS %lld: Progressive -> Interlaced", buf->start);
+
                 /* Store picture flags for later use by filters */
                 buf->flags = m->info->display_picture->flags;
                 
@@ -204,6 +299,16 @@
 {
     *width  = m->width;
     *height = m->height;
+    if( (m->info->display_picture->flags & PROGRESSIVE) && (m->height == 480) )
+      {
+          /* The frame is progressive and it's NTSC DVD height, so change its FPS to 23.976.
+             This might not be correct for the title. It's really just for scan.c's benefit.
+             Scan.c will reset the fps to 29.97, until a simple majority of the preview
+             frames report at 23.976.
+          */
+          //hb_log("Detecting NTSC Progressive Frame");
+          m->rate = 1126125;
+      }
     *rate   = m->rate;
     *aspect_ratio = m->aspect_ratio;
 }
Index: libhb/scan.c
===================================================================
--- libhb/scan.c	(revision 783)
+++ libhb/scan.c	(working copy)
@@ -290,6 +290,7 @@
     hb_buffer_t   * buf_ps, * buf_es, * buf_raw;
     hb_list_t     * list_es, * list_raw;
     hb_libmpeg2_t * mpeg2;
+    int progressive_count = 0;

     buf_ps   = hb_buffer_init( HB_DVD_READ_BUFFER_SIZE );
     list_es  = hb_list_init();
@@ -306,6 +307,8 @@
         FILE * file_preview;
         char   filename[1024];

+        //hb_log("Seeking to: %f", (float) ( i + 1 ) / 11.0 );
+        
         if (data->dvd)
         {
           if( !hb_dvd_seek( data->dvd, (float) ( i + 1 ) / 11.0 ) )
@@ -377,13 +380,40 @@
             goto error;
         }

-        if( !i )
+        /* Get size and rate infos */
+        title->rate = 27000000;
+        int ar;
+        hb_libmpeg2_info( mpeg2, &title->width, &title->height,
+                          &title->rate_base, &ar );
+        
+        if (title->rate_base == 1126125)
         {
-            /* Get size and rate infos */
-            title->rate = 27000000;
-            int ar;
-            hb_libmpeg2_info( mpeg2, &title->width, &title->height,
-                              &title->rate_base, &ar );
+            /* Frame FPS is 23.976 (meaning it's progressive), so
+               start keeping track of how many are reporting at
+               that speed. When enough show up that way, we want
+               to make that the overall title FPS.
+            */
+            progressive_count++;
+
+            if (progressive_count < 6)
+                /* Not enough frames are reporting as progressive,
+                   which means we should be conservative and use
+                   29.97 as the title's FPS for now.
+                */ 
+                title->rate_base = 900900;            
+            else
+            {
+                /* A majority of the scan frames are progressive. Make that
+                    the title's FPS, and announce it once to the log.
+                */
+                if (progressive_count == 6)
+                    hb_log("Title's mostly progressive NTSC, setting fps to 23.976");
+                title->rate_base = 1126125;                
+            }
+        }
+                
+        if( i == 2) // Use the third frame's info, so as to skip opening logos
+        {
             // The aspect ratio may have already been set by parsing the VOB/IFO details on a DVD, however
             // if we're working with program/transport streams that data needs to come from within the stream.
             if (title->aspect <= 0)

[jbrjake]

What if we could just treat all NTSC material as 29.97 and run --detelecine on it all?

Here's a patch I whipped up which seems to correct the problems with pullup (I need to talk to huevos_rancheros before I'd commit changes to his filters) -- no more 18fps jerkiness with 24fps content, no more duped frames keeping hard telecined material at 30fps instead of 24fps where it belongs.

It seems to handle all my NTSC content. Even difficult stuff like Buffy and Angel's pilots or the episode of Brisco County Jr. where the soft-telecine flags are set wrong.

http://pastebin.ca/683902

[dynaflash]

jbrjake, in attempting to do the testing you requested, I realized that the MacGui has a sanity check in it that won't let you choose detelecine on mostly progressive content. I couldn't reach you on irc so I went ahead and removed the sanity check in the macgui and added that to your patch.

So, if this is to be tested using the macgui, here is the same patch but with the necessary modifications to Controller.mm and PictureController.mm:

http://pastebin.ca/685941

Hope you don't mind

[jbrjake]

I think I've got something that does variable frame rate mixed content NTSC smoothly now. But it's still pretty crude. And it leaks like a [Censored]. And it really needs someone with experience in sync.c to fix the audio...hint, hint, eddyg =)

Right now, for speed, I only have it working with MP4 and ffmpeg encoding, but it's readily expandable to the other encoders, and I believe it might be possible in mkv too...though saintdev would be the one to know.

I've been testing with rhester's ipod_killer.vob. The way this works, btw, you have to force the framerate to 29.97 so ivtc can see all the frames. So don't leave it as "Same as source"...

So to sum up: use ffmpeg for the encoder, mp4 for the container, 29.97 for the framerate, apply the detelecine filter, and use mixed-speed NTSC content =)

I tried to heavily comment the diff:

EDIT: added in the muxmp4.c diff, which I accidentally forgot to save when I originally posted this last night.

Code: Select all

Index: libhb/detelecine.c
===================================================================
--- libhb/detelecine.c	(revision 964)
+++ libhb/detelecine.c	(working copy)
@@ -953,6 +953,17 @@
     {
         parity = 0;
     }
+    if ( ( buf_in->flags & PIC_FLAG_PROGRESSIVE_FRAME) )
+    {
+        /* I don't know why this is necessary. I
+           originally added it because I thought
+           that's what libmpcodec's IMGFIELD_ORDERED
+           flag meant, but it isn't...
+           Still, if you remove it, frames 
+           won't be discarded properly. ?!        */
+        parity = 0;
+    }
+    
 	pullup_submit_field( ctx, buf, parity );
 	pullup_submit_field( ctx, buf, parity^1 );
     if( buf_in->flags & PIC_FLAG_REPEAT_FIRST_FIELD )
@@ -975,7 +986,7 @@
         }
         else
         {
-            goto output_frame;
+            goto discard_frame;
         }
     }
     
@@ -987,7 +998,7 @@
         
 		if (!frame) 
         {
-            goto output_frame;
+            goto discard_frame;
         }
 		if( frame->length < 2 ) 
         {
@@ -995,19 +1006,19 @@
 
 			if( !(buf_in->flags & PIC_FLAG_REPEAT_FIRST_FIELD) )
             {
-                goto output_frame;
+                goto discard_frame;
             }
 			
             frame = pullup_get_frame( ctx );
 			
             if( !frame ) 
             {
-                goto output_frame;
+                goto discard_frame;
             }
 			if( frame->length < 2 ) 
             {
 				pullup_release_frame( frame );
-                goto output_frame;
+                goto discard_frame;
 			}
 		}
     }
@@ -1034,6 +1045,14 @@
 output_frame:    
     *buf_out = pv->buf_out;    
     return FILTER_OK;
+
+/* This and all discard_frame calls shown above are
+   the result of me restoring the functionality in
+   pullup that huevos_rancheros disabled because
+   HB couldn't handle it.                           */
+discard_frame:
+    *buf_out = pv->buf_out;
+    return FILTER_DROP;
 }
 
 
Index: libhb/muxmp4.c
===================================================================
--- libhb/muxmp4.c	(revision 964)
+++ libhb/muxmp4.c	(working copy)
@@ -380,7 +380,42 @@
         /* Because we use the audio samplerate as the timescale,
            we have to use potentially variable durations so the video
            doesn't go out of sync */
-        duration    = ( buf->stop * job->arate / 90000 ) - m->sum_dur;
+           
+        /* Okay so this is really crude. There's a comment titer left
+           right above this code about how QuickTime goes out of sync
+           if he doesn't vary the video duration by one unit every
+           frame to match the audio. (I want to change this eventually
+           anyway because I doubt QT has the same limitations anymore
+           and it's annoying not using a standard 90000 clockrate like
+           the rest of the world.)
+           
+           So anyway, what I'm doing here is setting the duration to
+           23.976 length if the frame is NTSC and the originating
+           MPEG-2 picture was marked progressive. And I'm *also* setting
+           it to 23.976 length if this new integer I added to the
+           hb_buffer_t, a token I call "ivtc", has been set to 1.
+           That happens in render.c, based on what happens in
+           detelecine.c...read on for that. If neither of those
+           conditions apply, the duration is set to 29.97 length.
+           
+           I'm not varying the lengths, which is why this bad
+           as a final way of handling this.                             */ 
+           if (buf->flags & 16)
+           {
+               duration = 1840;
+           }
+           else if (buf->ivtc == 1)
+           {
+                hb_log("progressive/ivtc frame");
+                duration    = 1840;
+           }
+           else if (buf->ivtc == 0)
+           {
+               hb_log("interlaced frame");
+               duration     = 1470;
+           }
+
+
         m->sum_dur += duration;
     }
     else
Index: libhb/encavcodec.c
===================================================================
--- libhb/encavcodec.c	(revision 964)
+++ libhb/encavcodec.c	(working copy)
@@ -204,7 +204,12 @@
         /* Write stats */
         fprintf( pv->file, "%s", pv->context->stats_out );
     }
+    
+    /* Copies over some info for variable frame rate muxing. */
+    buf->flags = in->flags;
+    buf->ivtc = in->ivtc;
 
+
     *buf_out = buf;
 
     return HB_WORK_OK;
Index: libhb/render.c
===================================================================
--- libhb/render.c	(revision 964)
+++ libhb/render.c	(working copy)
@@ -9,6 +9,9 @@
 #include "ffmpeg/avcodec.h"
 #include "ffmpeg/swscale.h"
 
+/* Used for keeping track of when frames are dropped by detelecine. */
+int ivtc_cadence[12];
+
 struct hb_work_private_s
 {
     hb_job_t * job;
@@ -184,6 +187,15 @@
     /* Setup render buffer */
     hb_buffer_t * buf_render = hb_buffer_init( 3 * job->width * job->height / 2 );  
 
+    /*  Rotate the cadence tracking. */
+    int i = 0;
+    for(i=11; i > 0; i--)
+    {
+        ivtc_cadence[i] = ivtc_cadence[i-1];
+        //hb_log("CADENCE: %i = %i", i, ivtc_cadence[i]);
+    }
+
+
     /* Apply filters */
     if( job->filters )
     {
@@ -217,6 +229,7 @@
             if( result == FILTER_OK )
             {
                 buf_tmp_in = buf_tmp_out;
+                ivtc_cadence[0] = 0;
             }
             else if( result == FILTER_DELAY )
             {
@@ -227,9 +240,29 @@
             {
                 hb_fifo_get( pv->subtitle_queue );
                 buf_tmp_in = NULL;
+                /* A drop means detelecine's latched its teeth onto
+                   something and has duped frames to discard.
+                   Mark this down...it means the surrounding frames
+                   should be played with a 23.976 duration.         */
+                ivtc_cadence[0] = 1;
                 break;
             }
         }
+
+        if ( (ivtc_cadence[1] == 1)  ||  (ivtc_cadence[2] == 1)  ||  (ivtc_cadence[3] == 1)  ||  (ivtc_cadence[4] == 1) )        
+        {
+            /* One of the last 4 frames was ditched by detelecine. That's
+               telecined stuff, and the frames need to be displayed
+               with a a 23.976 duration, so mark it down in the buffer
+               for use down the line in the muxers.                         */
+            hb_log("CADENCE: 3:2 pattern");
+            in->ivtc = 1;
+        }
+        else
+        {
+            in->ivtc = 0;
+        }
+        
     }   
         
     /* Apply subtitles */
@@ -267,6 +300,9 @@
         
         buf_tmp_in = buf_render;
     }  
+    
+    /* Pass that the frame might be telecined to the video encoder. */
+    buf_render->ivtc = in->ivtc;
 
     /* Set output to render buffer */
     (*buf_out) = buf_render;
Index: libhb/internal.h
===================================================================
--- libhb/internal.h	(revision 964)
+++ libhb/internal.h	(working copy)
@@ -50,7 +50,11 @@
 #define HB_FRAME_REF    0xF0
     uint8_t       frametype;
     uint8_t       flags;
-
+    
+    /* Keeps track of whether supposedly 29.976-length frames
+       should be displayed instead at 23.976 because of detelecine. */
+    int           ivtc;
+    
     /* Holds the output PTS from x264, for use by b-frame offsets in muxmp4.c */
     int64_t     renderOffset;
 

[eddyg]

Hi Jon,

Any audio sync issues won't be in sync.c, they will be in the muxing where you are setting the frame duration. If this is wrong the audio will get out of sync (as you say in your comments).

You are dropping the frame after the syncing, so it can't be an issue there.

What you have done in the muxing code makes sense, as long as the detelecine is dropping the frames, otherwise it wouldn't work so well.

I'm also suspicious of what will happen with audio syncing when the frame rate is changing all the time.

Cheers, Ed.