[Dark Shikari]

Patch review for open-GOP patch.

1. Use spaces, not tabs. I don't want to have to correct this for you.

2. Use consistent style. x264 puts the bracket on the next line, not the same line as the if statement.

3. What is this?

if( h->param.b_open_gop && h->param.i_keyint_max < 250 ) { //using default as cutoff        
        h->param.i_keyint_max = 250;
    }

This doesn't make any sense.

4. Don't add extra spaces for no reason:

@@ -1472,9 +1479,17 @@ int     x264_encoder_encode( x264_t *h,
         return 0;
     }
 
+
     if( h->fenc->i_type == X264_TYPE_IDR )

5. What is this for?

+    if( frames[1]->i_frame - h->frames.i_last_i >= h->param.i_keyint_max_i && h->param.b_open_gop )
+    {
+        frames[1]->i_type = X264_TYPE_I;
+        frames[1]->i_is_gop_start = 1;
+        return;        
+    }    
+
     if( num_frames == 1 )
     {
 no_b_frames:
         frames[1]->i_type = X264_TYPE_P;
+        if( frames[1]->i_frame - h->frames.i_last_i >= h->param.i_keyint_max_i && h->param.b_open_gop )
+        {
+            frames[1]->i_type = X264_TYPE_I;
+            frames[1]->i_is_gop_start = 2;
+            return;        
+        }    
         if( h->param.i_scenecut_threshold && scenecut( h, &a, frames, 0, 1 ) )
             frames[1]->i_type = idr_frame_type;
         return;
@@ -516,6 +530,12 @@ no_b_frames:
     {
         int num_bframes;
         int max_bframes = X264_MIN(num_frames-1, h->param.i_bframe);
+        if( frames[1]->i_frame - h->frames.i_last_i >= h->param.i_keyint_max_i && h->param.b_open_gop )
+        {
+            frames[1]->i_type = X264_TYPE_I;
+            frames[1]->i_is_gop_start = 3;
+            return;
+        }    
         if( h->param.i_scenecut_threshold && scenecut( h, &a, frames, 0, 1 ) )
         {
             frames[1]->i_type = idr_frame_type;
@@ -526,6 +546,12 @@ no_b_frames:
 
         for( j = 1; j < num_bframes+1; j++ )
         {
+            if( frames[j]->i_frame - h->frames.i_last_i >= h->param.i_keyint_max_i && h->param.b_open_gop )
+            {
+                frames[j]->i_type = X264_TYPE_I;
+                frames[j]->i_is_gop_start = 4;
+                return;        
+            }    
             if( h->param.i_scenecut_threshold && scenecut( h, &a, frames, j, j+1 ) )
             {
                 frames[j]->i_type = X264_TYPE_P;
@@ -533,6 +559,12 @@ no_b_frames:
             }
             frames[j]->i_type = X264_TYPE_B;
         }
+        if( frames[num_bframes+1]->i_frame - h->frames.i_last_i >= h->param.i_keyint_max_i && h->param.b_open_gop )
+        {
+            frames[num_bframes+1]->i_type = X264_TYPE_I;
+            frames[num_bframes+1]->i_is_gop_start = 5;
+            return;        
+        }    
         frames[num_bframes+1]->i_type = X264_TYPE_P;
     }
     else if( h->param.i_bframe_adaptive == X264_B_ADAPT_FAST )
@@ -552,12 +584,22 @@ no_b_frames:
         #define INTER_THRESH 300
         #define P_SENS_BIAS (50 - h->param.i_bframe_bias)
         frames[1]->i_type = X264_TYPE_B;
-
+        if( frames[1]->i_frame - h->frames.i_last_i >= h->param.i_keyint_max_i && h->param.b_open_gop )
+        {
+            frames[1]->i_type = X264_TYPE_I;
+            frames[1]->i_is_gop_start = 6;
+            return;        
+        }
         for( j = 2; j <= X264_MIN( h->param.i_bframe, num_frames-1 ); j++ )
         {
             int pthresh = X264_MAX(INTER_THRESH - P_SENS_BIAS * (j-1), INTER_THRESH/10);
             int pcost = x264_slicetype_frame_cost( h, &a, frames, 0, j+1, j+1, 1 );
-
+            if( frames[j]->i_frame - h->frames.i_last_i >= h->param.i_keyint_max_i && h->param.b_open_gop )
+            {
+                frames[j]->i_type = X264_TYPE_I;
+                frames[j]->i_is_gop_start = 7;
+                return;        
+            }
             if( pcost > pthresh*i_mb_count || frames[j+1]->i_intra_mbs[j+1] > i_mb_count/3 )
             {
                 frames[j]->i_type = X264_TYPE_P;
@@ -570,6 +612,12 @@ no_b_frames:
     else
     {
         int max_bframes = X264_MIN(num_frames-1, h->param.i_bframe);
+        if( frames[1]->i_frame - h->frames.i_last_i >= h->param.i_keyint_max_i && h->param.b_open_gop )
+        {
+            frames[1]->i_type = X264_TYPE_I;
+            frames[1]->i_is_gop_start = 8;
+            return;        
+        }    
         if( h->param.i_scenecut_threshold && scenecut( h, &a, frames, 0, 1 ) )
         {
             frames[1]->i_type = idr_frame_type;
@@ -578,6 +626,12 @@ no_b_frames:
 
         for( j = 1; j < max_bframes+1; j++ )
         {
+            if( frames[j]->i_frame - h->frames.i_last_i >= h->param.i_keyint_max_i && h->param.b_open_gop )
+            {
+                frames[j]->i_type = X264_TYPE_I;
+                frames[j]->i_is_gop_start = 9;
+                return;        
+            }    
             if( h->param.i_scenecut_threshold && scenecut( h, &a, frames, j, j+1 ) )
             {
                 frames[j]->i_type = X264_TYPE_P;
@@ -585,6 +639,12 @@ no_b_frames:
             }
             frames[j]->i_type = X264_TYPE_B;
         }
+        if( frames[max_bframes+1]->i_frame - h->frames.i_last_i >= h->param.i_keyint_max_i && h->param.b_open_gop )
+        {
+            frames[max_bframes+1]->i_type = X264_TYPE_I;
+            frames[max_bframes+1]->i_is_gop_start = 10;
+            return;        
+        }    
         frames[max_bframes+1]->i_type = X264_TYPE_P;
     }
 }

That's loads of code duplication, and I don't see why opengop should have code in slicetype decision at all. Why do we need the same code run 10 times with 10 different values for i_is_gop_start?

6. Why do we have two parameters that do the same thing?

     int         i_keyint_max;       /* Force an IDR keyframe at this interval */
+    int         i_keyint_max_i;