- Move frame rate code from sync to the end of render so it can account for frame timing changes made by filters.

 - Fix a bug that would make CFR alternate between massive drops and massive dups on
some titles.
 - If we're not doing CFR add a factor-of-two fudge factor to init_delay to account f
or the awful clock resolution of some mkvs and mp4s.


git-svn-id: svn://svn.handbrake.fr/HandBrake/trunk@2368 b64f7644-9d1e-0410-96f1-a4d463321fa5

5 files changed, 230 insertions, 134 deletions

diff --git a/libhb/common.h b/libhb/common.h
index 1c58286fe..60ba93ef6 100644
--- a/libhb/common.h
+++ b/libhb/common.h
@@ -183,9 +183,7 @@ struct hb_job_s
                             except with x264, to use its real QP/RF scale
          vbitrate:          output bitrate (kbps)
          vrate, vrate_base: output framerate is vrate / vrate_base
-         vfr:               boolean for variable frame rate detelecine
-         cfr:               boolean to use constant frame rates instead of
-                            passing through the source's frame durations.
+         cfr:               0 (vfr), 1 (cfr), 2 (pfr) [see render.c]
          pass:              0, 1 or 2 (or -1 for scan)
          h264_level:        vestigial boolean to decide if we're encoding for iPod
          crf:               boolean for whether to use constant rate factor with x264
diff --git a/libhb/encx264.c b/libhb/encx264.c
index 0bc1546ff..360dc8448 100644
--- a/libhb/encx264.c
+++ b/libhb/encx264.c
@@ -50,6 +50,9 @@ struct hb_work_private_s
     x264_picture_t   pic_in;
     uint8_t         *x264_allocated_pic;
 
+    uint32_t       frames_in;
+    uint32_t       frames_out;
+    uint32_t       frames_split; // number of frames we had to split
     int            chap_mark;   // saved chap mark when we're propagating it
     int64_t        last_stop;   // Debugging - stop time of previous input frame
     int64_t        init_delay;
@@ -346,10 +349,14 @@ int encx264Init( hb_work_object_t * w, hb_job_t * job )
         pv->init_delay += 2;
 
         /* For VFR, libhb sees the FPS as 29.97, but the longest frames
-           will use the duration of frames running at 23.976fps instead.. */
-        if (job->vfr)
+           will use the duration of frames running at 23.976fps instead.
+           Since detelecine occasionally makes mistakes and since we have
+           to deal with some really horrible timing jitter from mkvs and
+           mp4s encoded with low resolution clocks, make the delay very
+           conservative if we're not doing CFR. */
+        if ( job->cfr != 1 )
         {
-            pv->init_delay = 7506;
+            pv->init_delay *= 2;
         }
 
         /* The delay is 1 frames for regular b-frames, 2 for b-pyramid. */
@@ -363,6 +370,12 @@ int encx264Init( hb_work_object_t * w, hb_job_t * job )
 void encx264Close( hb_work_object_t * w )
 {
     hb_work_private_t * pv = w->private_data;
+
+    if ( pv->frames_split )
+    {
+        hb_log( "encx264: %u frames had to be split (%u in, %u out)",
+                pv->frames_split, pv->frames_in, pv->frames_out );
+    }
     /*
      * Patch the x264 allocated data back in so that x264 can free it
      * we have been using our own buffers during the encode to avoid copying.
@@ -606,6 +619,7 @@ int encx264Work( hb_work_object_t * w, hb_buffer_t ** buf_in,
             hb_buffer_t *buf = nal_encode( w, &pic_out, i_nal, nal );
             if ( buf )
             {
+                ++pv->frames_out;
                 if ( last_buf == NULL )
                     *buf_out = buf;
                 else
@@ -624,6 +638,7 @@ int encx264Work( hb_work_object_t * w, hb_buffer_t ** buf_in,
     }
 
     // Not EOF - encode the packet & wrap it in a NAL
+    ++pv->frames_in;
 
     // if we're re-ordering frames, check if this frame is too large to reorder
     if ( pv->init_delay && in->stop - in->start > pv->init_delay )
@@ -639,6 +654,7 @@ int encx264Work( hb_work_object_t * w, hb_buffer_t ** buf_in,
         // error. We take advantage of the fact that x264 buffers frame
         // data internally to feed the same image into the encoder multiple
         // times, just changing its start & stop times each time.
+        ++pv->frames_split;
         int64_t orig_stop = in->stop;
         int64_t new_stop = in->start;
         hb_buffer_t *last_buf = NULL;
@@ -665,6 +681,7 @@ int encx264Work( hb_work_object_t * w, hb_buffer_t ** buf_in,
             hb_buffer_t *buf = x264_encode( w, in );
             if ( buf )
             {
+                ++pv->frames_out;
                 if ( last_buf == NULL )
                     *buf_out = buf;
                 else
@@ -676,6 +693,7 @@ int encx264Work( hb_work_object_t * w, hb_buffer_t ** buf_in,
     }
     else
     {
+        ++pv->frames_out;
         *buf_out = x264_encode( w, in );
     }
     return HB_WORK_OK;
diff --git a/libhb/render.c b/libhb/render.c
index 5c7809dc3..fefec7792 100644
--- a/libhb/render.c
+++ b/libhb/render.c
@@ -27,6 +27,11 @@ struct hb_work_private_s
     uint64_t             total_gained_time;
     int64_t              chapter_time;
     int                  chapter_val;
+    int                  count_frames;      // frames output so far
+    double               frame_rate;        // 90KHz ticks per frame (for CFR/PFR)
+    double               out_last_stop;     // where last frame ended (for CFR/PFR)
+    int                  drops;             // frames dropped (for CFR/PFR)
+    int                  dups;              // frames duped (for CFR/PFR)
 };
 
 int  renderInit( hb_work_object_t *, hb_job_t * );
@@ -197,6 +202,131 @@ static void ApplySub( hb_job_t * job, hb_buffer_t * buf,
     hb_buffer_close( _sub );
 }
 
+// delete the buffer 'out' from the chain of buffers whose head is 'buf_out'.
+// out & buf_out must be non-null (checks prior to anywhere this routine
+// can be called guarantee this) and out must be on buf_out's chain
+// (all places that call this get 'out' while traversing the chain).
+// 'out' is freed and its predecessor is returned.
+static hb_buffer_t *delete_buffer_from_chain( hb_buffer_t **buf_out, hb_buffer_t *out)
+{
+    hb_buffer_t *succ = out->next;
+    hb_buffer_t *pred = *buf_out;
+    if ( pred == out )
+    {
+        // we're deleting the first buffer
+        *buf_out = succ;
+    }
+    else
+    {
+        // target isn't the first buf so search for its predecessor.
+        while ( pred->next != out )
+        {
+            pred = pred->next;
+        }
+        // found 'out' - remove it from the chain
+        pred->next = succ;
+    }
+    hb_buffer_close( &out );
+    return succ;
+}
+
+// insert buffer 'succ' after buffer chain element 'pred'.
+// caller must guarantee that 'pred' and 'succ' are non-null.
+static hb_buffer_t *insert_buffer_in_chain( hb_buffer_t *pred, hb_buffer_t *succ )
+{
+    succ->next = pred->next;
+    pred->next = succ;
+    return succ;
+}
+
+// This section of the code implements video frame rate control.
+// Since filters are allowed to duplicate and drop frames (which
+// changes the timing), this has to be the last thing done in render.
+//
+// There are three options, selected by the value of job->cfr:
+//   0 - Variable Frame Rate (VFR) or 'same as source': frame times
+//       are left alone
+//   1 - Constant Frame Rate (CFR): Frame timings are adjusted so that all
+//       frames are exactly job->vrate_base ticks apart. Frames are dropped
+//       or duplicated if necessary to maintain this spacing.
+//   2 - Peak Frame Rate (PFR): job->vrate_base is treated as the peak
+//       average frame rate. I.e., the average frame rate (current frame
+//       end time divided by number of frames so far) is never allowed to be
+//       greater than job->vrate_base and frames are dropped if necessary
+//       to keep the average under this value. Other than those drops, frame
+//       times are left alone.
+//
+
+static void adjust_frame_rate( hb_work_private_t *pv, hb_buffer_t **buf_out )
+{
+    hb_buffer_t *out = *buf_out;
+
+    while ( out && out->size > 0 )
+    {
+        // this frame has to start where the last one stopped.
+        out->start = pv->out_last_stop;
+
+        // compute where this frame would stop if the frame rate were constant
+        // (this is our target stopping time for CFR and earliest possible
+        // stopping time for PFR).
+        double cfr_stop = pv->frame_rate * ( pv->count_frames + 1 );
+
+        if ( cfr_stop - (double)out->stop >= pv->frame_rate )
+        {
+            // This frame stops a frame time or more in the past - drop it
+            // but don't lose its chapter mark.
+            if ( out->new_chap )
+            {
+                pv->chapter_time = out->start;
+                pv->chapter_val = out->new_chap;
+            }
+            ++pv->drops;
+            out = delete_buffer_from_chain( buf_out, out );
+            continue;
+        }
+
+        // at this point we know that this frame doesn't push the average
+        // rate over the limit so we just pass it on for PFR. For CFR we're
+        // going to return it (with its start & stop times modified) and
+        // we may have to dup it.
+        ++pv->count_frames;
+        if ( pv->job->cfr > 1 )
+        {
+            // PFR - we're going to keep the frame but may need to
+            // adjust it's stop time to meet the average rate constraint.
+            if ( out->stop <= cfr_stop )
+            {
+                out->stop = cfr_stop;
+            }
+        }
+        else
+        {
+            // we're doing CFR so we have to either trim some time from a
+            // buffer that ends too far in the future or, if the buffer is
+            // two or more frame times long, split it into multiple pieces,
+            // each of which is a frame time long.
+            double excess_dur = (double)out->stop - cfr_stop;
+            out->stop = cfr_stop;
+            for ( ; excess_dur >= pv->frame_rate; excess_dur -= cfr_stop )
+            {
+                /* next frame too far ahead - dup current frame */
+                hb_buffer_t *dup = hb_buffer_init( out->size );
+                memcpy( dup->data, out->data, out->size );
+                hb_buffer_copy_settings( dup, out );
+                dup->new_chap = 0;
+                dup->start = cfr_stop;
+                cfr_stop += pv->frame_rate;
+                dup->stop = cfr_stop;
+                out = insert_buffer_in_chain( out, dup );
+                ++pv->dups;
+                ++pv->count_frames;
+            }
+        }
+        pv->out_last_stop = out->stop;
+        out = out->next;
+    }
+}
+
 int renderWork( hb_work_object_t * w, hb_buffer_t ** buf_in,
                 hb_buffer_t ** buf_out )
 {
@@ -217,7 +347,7 @@ int renderWork( hb_work_object_t * w, hb_buffer_t ** buf_in,
          * the correct order, and add the end of stream buffer to the
          * end.
          */     
-        while( next = hb_fifo_get( pv->delay_queue ) )
+        while( ( next = hb_fifo_get( pv->delay_queue ) ) != NULL )
         {
             
             /* We can't use the given time stamps. Previous frames
@@ -242,6 +372,10 @@ int renderWork( hb_work_object_t * w, hb_buffer_t ** buf_in,
         {
             tail->next = in;
             *buf_out = head;
+            if ( job->cfr )
+            {
+                adjust_frame_rate( pv, buf_out );
+            }
         } else {
             *buf_out = in;
         }     
@@ -270,7 +404,7 @@ int renderWork( hb_work_object_t * w, hb_buffer_t ** buf_in,
     }
 
     /* If there's a chapter mark remember it in case we delay or drop its frame */
-    if( in->new_chap && job->vfr )
+    if( in->new_chap )
     {
         pv->chapter_time = in->start;
         pv->chapter_val = in->new_chap;
@@ -321,33 +455,26 @@ int renderWork( hb_work_object_t * w, hb_buffer_t ** buf_in,
             }
             else if( result == FILTER_DROP )
             {
-                if( job->vfr )
-                {
-                    /* We need to compensate for the time lost by dropping this frame.
-                       Spread its duration out in quarters, because usually dropped frames
-                       maintain a 1-out-of-5 pattern and this spreads it out amongst the remaining ones.
-                       Store these in the lost_time array, which has 4 slots in it.
-                       Because not every frame duration divides evenly by 4, and we can't lose the
-                       remainder, we have to go through an awkward process to preserve it in the 4th array index. */
-                    uint64_t temp_duration = buf_tmp_out->stop - buf_tmp_out->start;
-                    pv->lost_time[0] += (temp_duration / 4);
-                    pv->lost_time[1] += (temp_duration / 4);
-                    pv->lost_time[2] += (temp_duration / 4);
-                    pv->lost_time[3] += ( temp_duration - (temp_duration / 4) - (temp_duration / 4) - (temp_duration / 4) );
-
-                    pv->total_lost_time += temp_duration;
-                    pv->dropped_frames++;
-
-                    /* Pop the frame's subtitle and dispose of it. */
-                    hb_buffer_t * subtitles = hb_fifo_get( pv->subtitle_queue );
-                    hb_buffer_close( &subtitles );
-                    buf_tmp_in = NULL;
-                    break;
-                }
-                else
-                {
-                    buf_tmp_in = buf_tmp_out;
-                }
+                /* We need to compensate for the time lost by dropping this frame.
+                   Spread its duration out in quarters, because usually dropped frames
+                   maintain a 1-out-of-5 pattern and this spreads it out amongst the remaining ones.
+                   Store these in the lost_time array, which has 4 slots in it.
+                   Because not every frame duration divides evenly by 4, and we can't lose the
+                   remainder, we have to go through an awkward process to preserve it in the 4th array index. */
+                uint64_t temp_duration = buf_tmp_out->stop - buf_tmp_out->start;
+                pv->lost_time[0] += (temp_duration / 4);
+                pv->lost_time[1] += (temp_duration / 4);
+                pv->lost_time[2] += (temp_duration / 4);
+                pv->lost_time[3] += ( temp_duration - (temp_duration / 4) - (temp_duration / 4) - (temp_duration / 4) );
+
+                pv->total_lost_time += temp_duration;
+                pv->dropped_frames++;
+
+                /* Pop the frame's subtitle and dispose of it. */
+                hb_buffer_t * subtitles = hb_fifo_get( pv->subtitle_queue );
+                hb_buffer_close( &subtitles );
+                buf_tmp_in = NULL;
+                break;
             }
         }
     }
@@ -429,7 +556,7 @@ int renderWork( hb_work_object_t * w, hb_buffer_t ** buf_in,
         hb_buffer_copy_settings( buf_render, buf_tmp_in );
     }
 
-    if (*buf_out && job->vfr)
+    if (*buf_out )
     {
         hb_fifo_push( pv->delay_queue, *buf_out );
         *buf_out = NULL;
@@ -440,15 +567,12 @@ int renderWork( hb_work_object_t * w, hb_buffer_t ** buf_in,
      * two always in there should we need to rewrite the durations on them.
      */
 
-    if( job->vfr )
+    if( hb_fifo_size( pv->delay_queue ) >= 4 )
     {
-        if( hb_fifo_size( pv->delay_queue ) >= 4 )
-        {
-            *buf_out = hb_fifo_get( pv->delay_queue );
-        }
+        *buf_out = hb_fifo_get( pv->delay_queue );
     }
 
-    if( *buf_out && job->vfr)
+    if( *buf_out )
     {
         /* The current frame exists. That means it hasn't been dropped by a filter.
            Make it accessible as ivtc_buffer so we can edit its duration if needed. */
@@ -527,6 +651,10 @@ int renderWork( hb_work_object_t * w, hb_buffer_t ** buf_in,
 
     }
 
+    if ( buf_out && *buf_out && job->cfr )
+    {
+        adjust_frame_rate( pv, buf_out );
+    }
     return HB_WORK_OK;
 }
 
@@ -534,6 +662,12 @@ void renderClose( hb_work_object_t * w )
 {
     hb_work_private_t * pv = w->private_data;
 
+    if ( pv->job->cfr )
+    {
+        hb_log("render: %d frames output, %d dropped and %d duped for CFR/PFR",
+               pv->count_frames, pv->drops, pv->dups );
+    }
+
     hb_log("render: lost time: %lld (%i frames)", pv->total_lost_time, pv->dropped_frames);
     hb_log("render: gained time: %lld (%i frames) (%lld not accounted for)", pv->total_gained_time, pv->extended_frames, pv->total_lost_time - pv->total_gained_time);
     if (pv->dropped_frames)
@@ -599,6 +733,8 @@ int renderInit( hb_work_object_t * w, hb_job_t * job )
     pv->chapter_time = 0;
     pv->chapter_val  = 0;
 
+    pv->frame_rate = (double)job->vrate_base * (1./300.);
+
     /* Setup filters */
     /* TODO: Move to work.c? */
     if( job->filters )
diff --git a/libhb/sync.c b/libhb/sync.c
index f7b342b61..a1a7f1ad9 100644
--- a/libhb/sync.c
+++ b/libhb/sync.c
@@ -518,85 +518,27 @@ static void SyncVideo( hb_work_object_t * w )
             }
         }
 
-        int64_t duration;
-        if ( job->mux & HB_MUX_AVI || job->cfr )
-        {
-            /*
-             * The concept of variable frame rate video was a bit too advanced
-             * for Microsoft so AVI doesn't support it. Since almost all dvd
-             * video is VFR we have to convert it to constant frame rate to
-             * put it in an AVI container. So here we duplicate, drop and
-             * otherwise trash video frames to appease the gods of Redmond.
-             */
-
-            /* mpeg durations are exact when expressed in ticks of the
-             * 27MHz System clock but not in HB's 90KHz PTS clock. To avoid
-             * a truncation bias that will eventually cause the audio to desync
-             * we compute the duration of the next frame using 27MHz ticks
-             * then truncate it to 90KHz. */
-            duration = ( (int64_t)(pv->count_frames + 1 ) * job->vrate_base ) / 300 -
-                       pv->next_start;
-
-            /* We don't want the input & output clocks to be exactly in phase
-             * otherwise small variations in the time will cause us to think
-             * we're a full frame off & there will be lots of drops and dups.
-             * We offset the input clock by half the duration so it's maximally
-             * out of phase with the output clock. */
-            if( cur->start < pv->next_start  - ( duration >> 1 ) )
-            {
-                /* current frame too old - drop it */
-                if ( cur->new_chap )
-                {
-                    pv->chap_mark = cur->new_chap;
-                }
-                hb_buffer_close( &cur );
-                pv->cur = cur = hb_fifo_get( job->fifo_raw );
-                pv->next_pts = next->start;
-                ++pv->drops;
-                continue;
-            }
-
-            if( next->start > pv->next_start + duration + ( duration >> 1 ) )
-            {
-                /* next frame too far ahead - dup current frame */
-                buf_tmp = hb_buffer_init( cur->size );
-                hb_buffer_copy_settings( buf_tmp, cur );
-                memcpy( buf_tmp->data, cur->data, cur->size );
-                buf_tmp->sequence = cur->sequence;
-                ++pv->dups;
-            }
-            else
-            {
-                /* this frame in our time window & doesn't need to be duped */
-                buf_tmp = cur;
-                pv->cur = cur = hb_fifo_get( job->fifo_raw );
-                pv->next_pts = next->start;
-            }
-        }
-        else
+        /*
+         * Adjust the pts of the current frame so that it's contiguous
+         * with the previous frame. The start time of the current frame
+         * has to be the end time of the previous frame and the stop
+         * time has to be the start of the next frame.  We don't
+         * make any adjustments to the source timestamps other than removing
+         * the clock offsets (which also removes pts discontinuities).
+         * This means we automatically encode at the source's frame rate.
+         * MP2 uses an implicit duration (frames end when the next frame
+         * starts) but more advanced containers like MP4 use an explicit
+         * duration. Since we're looking ahead one frame we set the
+         * explicit stop time from the start time of the next frame.
+         */
+        buf_tmp = cur;
+        pv->cur = cur = hb_fifo_get( job->fifo_raw );
+        pv->next_pts = cur->start;
+        int64_t duration = cur->start - buf_tmp->start;
+        if ( duration <= 0 )
         {
-            /*
-             * Adjust the pts of the current frame so that it's contiguous
-             * with the previous frame. The start time of the current frame
-             * has to be the end time of the previous frame and the stop
-             * time has to be the start of the next frame.  We don't
-             * make any adjustments to the source timestamps other than removing
-             * the clock offsets (which also removes pts discontinuities).
-             * This means we automatically encode at the source's frame rate.
-             * MP2 uses an implicit duration (frames end when the next frame
-             * starts) but more advanced containers like MP4 use an explicit
-             * duration. Since we're looking ahead one frame we set the
-             * explicit stop time from the start time of the next frame.
-             */
-            buf_tmp = cur;
-            pv->cur = cur = hb_fifo_get( job->fifo_raw );
-            pv->next_pts = cur->start;
-            duration = cur->start - buf_tmp->start;
-            if ( duration <= 0 )
-            {
-                hb_log( "sync: invalid video duration %lld, start %lld, next %lld",
-                        duration, buf_tmp->start, next->start );
-            }
+            hb_log( "sync: invalid video duration %lld, start %lld, next %lld",
+                    duration, buf_tmp->start, next->start );
         }
 
         buf_tmp->start = pv->next_start;
diff --git a/libhb/work.c b/libhb/work.c
index 4cd9ffe6e..8e0734ceb 100644
--- a/libhb/work.c
+++ b/libhb/work.c
@@ -186,20 +186,19 @@ void hb_display_job_info( hb_job_t * job )
         hb_log( "     + bitrate %d kbps", title->video_bitrate / 1000 );
     }
     
-    if( job->vfr)
-    {
-        hb_log( "   + frame rate: %.3f fps -> variable fps",
-            (float) title->rate / (float) title->rate_base );
-    }
-    else if( !job->cfr )
+    if( !job->cfr )
     {
         hb_log( "   + frame rate: same as source (around %.3f fps)",
             (float) title->rate / (float) title->rate_base );
     }
     else
     {
-        hb_log( "   + frame rate: %.3f fps -> constant %.3f fps",
-            (float) title->rate / (float) title->rate_base, (float) job->vrate / (float) job->vrate_base );
+        static const char *frtypes[] = {
+            "", "constant", "peak rate limited to"
+        };
+        hb_log( "   + frame rate: %.3f fps -> %s %.3f fps",
+            (float) title->rate / (float) title->rate_base, frtypes[job->cfr],
+            (float) job->vrate / (float) job->vrate_base );
     }
 
     if( job->anamorphic.mode )
@@ -402,16 +401,19 @@ static void do_job( hb_job_t * job, int cpu_count )
         hb_log( "New dimensions %i * %i", job->width, job->height );
     }
 
-    if( ( job->mux & HB_MUX_AVI ) || job->cfr )
+    if( job->mux & HB_MUX_AVI )
     {
-        /* VFR detelecine is not compatible with AVI or constant frame rates. */
-        job->vfr = 0;
+        // The concept of variable frame rate video was a bit too advanced
+        // for Microsoft so AVI doesn't support it. Since almost all dvd
+        // video is VFR we have to convert it to constant frame rate to
+        // put it in an AVI container. So duplicate, drop and
+        // otherwise trash video frames to appease the gods of Redmond.
+        job->cfr = 1;
     }
 
-    if ( job->vfr )
+    if ( job->cfr == 0 )
     {
-        /* Ensure we're using "Same as source" FPS,
-           aka VFR, if we're doing VFR detelecine. */
+        /* Ensure we're using "Same as source" FPS */
         job->vrate_base = title->rate_base;
     }