open-gop patch for x264 (committed)

[Trahald]

I hacked up a open gop patch. all it really does is change the idr interval higher and force i-frames in at the max-keyint and flag them as recovery points. then clears the reference list of all frames behind the i when the first non-b is ready to be encoded. So for the most part it just interferes with frame decisioning a little and does some reference maintenance. x264 already supported i (small i) frames so it was fairly easy.

in a very few tests i get about 1.5% smaller size. thats with both at b-adapt 1(x264 default). B-adapt 2 with open-gop gives 4% improvement over a closed gop encode at b-adapt 1 and 5% over b-adapt 2 with closed-gop. fades seem to do well bitrate wise also.

This patch really is only useful if you have small gops (ie bluray) . my tests were at min keyint of 12. This patch is not for your production encodes as it is experimental. Do not create any binaries with this patch that may be in automatic upgrading scripts to protect the innocent.

there will be 2 attachments. one is the patch by itself and one with hrd(interlaced) 13 patch on top of it (as hrd has to be adjusted to open gop and also the patches update the same part of encoder.c ). this hrd version does not replace the current hrd patch.

again both are experimental

[Dark Shikari]

Originally Posted by Trahald

I hacked up a open gop patch. all it really does is change the idr interval higher and force i-frames in at the max-keyint and flag them as recovery points. then clears the reference list of all frames behind the i when the first non-b is ready to be encoded. So for the most part it just interferes with frame decisioning a little and does some reference maintenance. x264 already supported i (small i) frames so it was fairly easy.

Did you make it so scenecuts are closed-gop but I-frames at max-keyint (not scenecuts) are open gop?

[Trahald]

Originally Posted by Dark Shikari

Did you make it so scenecuts are closed-gop but I-frames at max-keyint (not scenecuts) are open gop?

yessir. my thought was that max keyint is some arbitrary thing, but a scenecut is x264 saying a key frame is needed here (re:quality). i plan to go back in and look for more cases where open gop makes sense.

[techouse]

interesting.....

[shon3i]

Can somebody compile with opengop feature and x264_win_zone_parse_fix_05.diff and x264_hrd_pulldown.13_interlace.diff

[moviefan]

Also AutoVAQ.02 in addition to shon3i's proposal...?

[Trahald]

This is an experimental build with Open GOP for those that want to play w/it.

http://www.mediafire.com/file/4muyzw...71_opengop.zip

fprofiled

x264_open_gop_hrd.2.diff
x264_win_zone_parse_fix_05.diff
x264_AutoVAQ.03.diff

[rack04]

Originally Posted by Trahald

This is an experimental build with Open GOP for those that want to play w/it.

http://www.mediafire.com/file/4muyzw...71_opengop.zip

fprofiled

x264_open_gop_hrd.2.diff
x264_win_zone_parse_fix_05.diff
x264_AutoVAQ.03.diff

Thanks. Can you make a build with "x264_open_gop_hrd.2.diff " and "x264_hrd_pulldown.13_interlace.diff" for testing Blu-ray sources?

[nurbs]

I guess "x264_open_gop_hrd.2.diff" contains the "x264_hrd_pulldown.13_interlace.diff"

[rack04]

Originally Posted by nurbs

I guess "x264_open_gop_hrd.2.diff" contains the "x264_hrd_pulldown.13_interlace.diff"

According to Trahald:

Originally Posted by Trahald

there will be 2 attachments. one is the patch by itself and one with hrd(interlaced) 13 patch on top of it (as hrd has to be adjusted to open gop and also the patches update the same part of encoder.c ). this hrd version does not replace the current hrd patch.

[shon3i]

@Trahald, thanks for patch and experimental build, results are more than expected


With Open GOP

x264.exe" --pass 2 --bitrate 1000 --stats "drift.stats" --level 4.1 --keyint 24 --min-keyint 1 --ref 4 --mixed-refs --no-fast-pskip --bframes 3 --b-adapt 2 --weightb --direct auto --deblock -1:-1 --subme 9 --trellis 2 --psy-rd 1.0:0.2 --partitions p8x8,b8x8,i4x4,i8x8 --8x8dct --vbv-bufsize 17999 --vbv-maxrate 16000 --me umh --threads 12 --thread-input --aq-mode 0 --sar 1:1 --progress --no-dct-decimate --no-psnr --no-ssim --output "drift.264" "drift.avs" --sar 1:1 --mvrange 511 --aud --nal-hrd --open-gop

avis [info]: 720x528 @ 25.00 fps (986 frames)
x264 [info]: using SAR=1/1
x264 [info]: using cpu capabilities: MMX2 SSE2Fast FastShuffle SSEMisalign LZCNT
x264 [info]: profile High, level 4.1

x264 [info]: slice I:56 Avg QP:26.41 size: 16729
x264 [info]: slice P:321 Avg QP:28.84 size: 7113
x264 [info]: slice B:609 Avg QP:31.49 size: 2709
x264 [info]: consecutive B-frames: 4.3% 16.7% 40.1% 38.8%
x264 [info]: mb I I16..4: 30.9% 60.1% 9.1%
x264 [info]: mb P I16..4: 5.3% 5.4% 0.7% P16..4: 55.8% 8.5% 10.5% 0.0% 0.0% skip:13.7%
x264 [info]: mb B I16..4: 0.1% 0.2% 0.0% B16..8: 42.1% 0.9% 1.2% direct: 3.9% skip:51.5% L0:41.9% L1:51.5% BI: 6.6%
x264 [info]: 8x8 transform intra:55.4% inter:77.8%
x264 [info]: direct mvs spatial:89.2% temporal:10.8%
x264 [info]: coded y,uvDC,uvAC intra:54.6% 62.4% 22.4% inter:16.8% 32.3% 4.0%
x264 [info]: ref P L0 75.2% 15.3% 6.7% 2.8%
x264 [info]: ref B L0 86.0% 10.2% 3.9%
x264 [info]: kb/s:987.8
encoded 986 frames, 18.08 fps, 988.32 kb/s

Without Open GOP

x264.exe" --pass 2 --bitrate 1000 --stats "drift.stats" --level 4.1 --keyint 24 --min-keyint 1 --ref 4 --mixed-refs --no-fast-pskip --bframes 3 --b-adapt 2 --weightb --direct auto --deblock -1:-1 --subme 9 --trellis 2 --psy-rd 1.0:0.2 --partitions p8x8,b8x8,i4x4,i8x8 --8x8dct --vbv-bufsize 17999 --vbv-maxrate 16000 --me umh --threads 12 --thread-input --aq-mode 0 --sar 1:1 --progress --no-dct-decimate --no-psnr --no-ssim --output "drift.mkv" "drift.avs" --sar 1:1 --mvrange 511 --aud --nal-hrd

avis [info]: 720x528 @ 25.00 fps (986 frames)
x264 [info]: using SAR=1/1
x264 [info]: using cpu capabilities: MMX2 SSE2Fast FastShuffle SSEMisalign LZCNT
x264 [info]: profile High, level 4.1

x264 [info]: slice I:72 Avg QP:27.25 size: 14636
x264 [info]: slice P:364 Avg QP:29.19 size: 6626
x264 [info]: slice B:550 Avg QP:31.74 size: 2569
x264 [info]: consecutive B-frames: 10.4% 15.5% 37.7% 36.3%
x264 [info]: mb I I16..4: 33.6% 58.6% 7.8%
x264 [info]: mb P I16..4: 4.6% 4.7% 0.6% P16..4: 56.6% 8.0% 10.9% 0.0% 0.0% skip:14.6%
x264 [info]: mb B I16..4: 0.1% 0.2% 0.0% B16..8: 41.0% 0.9% 1.2% direct: 3.5% skip:53.0% L0:41.3% L1:52.1% BI: 6.6%
x264 [info]: 8x8 transform intra:54.9% inter:77.6%
x264 [info]: direct mvs spatial:88.9% temporal:11.1%
x264 [info]: coded y,uvDC,uvAC intra:53.1% 63.5% 24.5% inter:16.9% 32.2% 4.1%
x264 [info]: ref P L0 76.2% 15.1% 6.4% 2.3%
x264 [info]: ref B L0 88.2% 9.0% 2.8%
x264 [info]: kb/s:989.6
encoded 986 frames, 20.15 fps, 990.23 kb/s

and I have to ask how this patch is far from the final version because work perfectly

[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;

[Trahald]

@Dark Shikari
1,2 & 4 . the only cleanup i did was to remove fprintf's. I'll take care of that stuff tho

3 & 6. when open_gop is true then i_keyint_max ends up meaning (max IDR interval) which i push to 250 unless its value is higher. i_keyint_max_i represents the interval of little i frames. So...
i_keyint_max_i = i_keyint_max; // the users input
i_keyint_max = i_keyint_max > 250 ? i_keyint_max : 250;

5. yeah yuck. thats me throwing spaghetti at the wall and seeing what sticks. i'll work on it. the multiple values thing was more for debugging but since its harmless i left it.

[Dark Shikari]

Originally Posted by Trahald

3 & 6. when open_gop is true then i_keyint_max ends up meaning (max IDR interval) which i push to 250 unless its value is higher. i_keyint_max_i represents the interval of little i frames. So...
i_keyint_max_i = i_keyint_max; // the users input
i_keyint_max = i_keyint_max > 250 ? i_keyint_max : 250;

I don't think this is really a good solution.

When someone says they want a keyframe interval of 90 frames, they mean they want a keyframe every 90 frames; an open-GOP I-frame is still a keyframe, so that counts. Yes, we'll have to redefine what i_keyint_max does in the documentation to not literally say "IDR."

If, for technical reasons, we still need to force IDR frames every once in a while despite open-GOPs being perfectly fine keyframes, the value should be really large.

[Trahald]

Originally Posted by Dark Shikari

I don't think this is really a good solution.

When someone says they want a keyframe interval of 90 frames, they mean they want a keyframe every 90 frames; an open-GOP I-frame is still a keyframe, so that counts. Yes, we'll have to redefine what i_keyint_max does in the documentation to not literally say "IDR."

If, for technical reasons, we still need to force IDR frames every once in a while despite open-GOPs being perfectly fine keyframes, the value should be really large.

only 1 IDR is needed in a stream. (well there i a limit on poc_msb but its too huge to ever be hit) keeping the value was helpful since x264 makes some non-gop size related decisions based on the value. but i can remove the extra integer and use the users input as max_keyint then either...
1. anyplace there is a max_keyint referenced that relates to scenecut, replace w/integer 400000 when open gop is true
2. remove max_keyint from the scenecut routines (w/or w/out opengop) and do whatever keyint is used for there some other way

[Dark Shikari]

Originally Posted by Trahald

keeping the value was helpful since x264 makes some non-gop size related decisions based on the value.

Which ones? Scenecut should use the time since the last keyframe, not the last IDR, for its decision.

Originally Posted by Trahald

1. anyplace there is a max_keyint referenced that relates to scenecut, replace w/integer 400000 when open gop is true
2. remove max_keyint from the scenecut routines (w/or w/out opengop) and do it some other way

Again, "max keyint" in scenecut routines should be the distance between keyframes, not the distance between IDR frames.

[Trahald]

Originally Posted by Dark Shikari

Which ones? Scenecut should use the time since the last keyframe, not the last IDR, for its decision.

All... Problem is I dont want x264 thinking 'i only have 24 frames to work with.' in my early tests default decisioning resulted in open gop being the same size or sometimes bigger than closed gop equivelant. I got the most benefit (ie output closer in size to a stream that had a large key interval , which was my goal) when x264 thought there was a big distance between frames.

Again, "max keyint" in scenecut routines should be the distance between keyframes, not the distance between IDR frames.

yeah.. i didnt really make my post clear.

in either scenario, max_keyint_i is done away with and there is only max key int
in scenarion 1. its a cosmetic removal as im just hardcoding the larger value. but.. this would code wise remove the extra value. max_keyint would be commented as maximum distance between IDR frames or Recovery points.

OR

scenario 2. here again, there is only one max_keyint . but here we remove any scenecut decisioning that relies on the distance between keyintervals, whatever that interval is. scenecuts in streams at max_keyint 24 would be in the same spots as streams with max_keyint 50, etc.

[Dark Shikari]

Originally Posted by Trahald

All... Problem is I dont want x264 thinking 'i only have 24 frames to work with.'

What do you mean? x264 only cares about the number of frames it has to "work with" in two places:

1. B-frame decision, in which this doesn't help, since you're still forced to end the GOP, even if it's on an open GOP.

2. Scenecut decision, in which what you are doing is outright lying to the scenecut algorithm about where the last keyframe actually was. I will not accept a patch that does this for any reason whatsoever. If you think that the scenecut algorithm is suboptimal, fix it separately.

Originally Posted by Trahald

in either scenario, max_keyint_i is done away with and there is only max key int
in scenarion 1. its a cosmetic removal as im just hardcoding the larger value.

Why have a larger value at all if you're not going to allow it to be adjusted? Why do we have to force IDR frames every once in a while?

Originally Posted by Trahald

scenario 2. here again, there is only one max_keyint . but here we remove any scenecut decisioning that relies on the distance between keyintervals, whatever that interval is. scenecuts in streams at max_keyint 24 would be in the same spots as streams with max_keyint 50, etc.

This is a bad idea and will reduce encoding efficiency.

What is wrong with my original suggestion of just doing it the sensible way--leaving scenecut as it is, and treating all open GOPs as keyframes? Why do we have to munge the scenecut algorithm like this, independent of what your patch is actually doing?

[Selur]

Again, "max keyint" in scenecut routines should be the distance between keyframes, not the distance between IDR frames.

how about two two parameters 'max IDR interval' and 'may key frame interval' ? (I need IDR frames to cut lossles, don't I?)

[Dark Shikari]

Originally Posted by Selur

how about two two parameters 'max IDR interval' and 'may key frame interval' ? (I need IDR frames to cut lossles, don't I?)

Well, you probably can cut at a recovery point, the cutting software just has to be smarter.