[cubicibo]

Internal:

• Each frame now conveys a duration.
• Slicetype determines the cpbRemovalDelay and dpbOutputDelay for HRD conformance.
• Slicetype places keyframes on the min/max interval in $ClockTick$ units rather than an absolute frame counts.
• Ratecontrol weights each frame base on its $cpbDuration$ and $frameDuration$.
• VBV Lookahead considers the frame duration.
• CuTree uses the $frameDuration$ to give more weight to long-standing frames/blocks.

User interface:

• --pic-struct and --frame-dup now produce conformant bitstreams.
• Clean-up interaction between both option, and reject values that would produce orphaned fields.
• Add --psfile to specify arbitrary pulldown patterns ("VFR-in-CFR" container).

Limitation:

• VFR encode (or with structures / de-dup) is not well suited with how --frames (totalFrames) is defined, because this does not convey a duration.

[cubicibo]

@mcw-Lavanya Ready for review.

I don't know why only some debug VC smoke test fails. The same test pass on non-debug binaries.
Furthermore, it's only the tests that specify --bitrate. I cannot build or debug Windows binaries so I cannot look into it.

[mcw-Lavanya]

Hi @cubicibo, thanks for the detailed writeup on this — really nice work threading frame duration through the whole ratecontrol/slicetype/cuTree chain.

I dug into the Windows Debug + --bitrate smoke test failures a bit, and I have a suspicion about where it's coming from. Wanted to flag it humbly since I could be missing context on your side.

In ratecontrol.cpp, the constructor (RateControl::RateControl()) used to have:

cpp
m_fps = (double)m_param->fpsNum / m_param->fpsDenom;

In commit bc7e03c ("ratecontrol: support arbitrary timebases for VFR"), this line looks like it was replaced with:

cpp
m_timebase = (double)m_param->fpsDenom / m_param->fpsNum;

But m_fps (declared in ratecontrol.h, no default initializer) still seems to be read in several places later in the file, e.g.:

ratecontrol.cpp:389 – X265_MIN(m_param->keyframeMax, (int)(m_fps + 0.5))
ratecontrol.cpp:845 – vbvMaxBitrate * (m_fps / m_param->reconfigWindowSize)
ratecontrol.cpp:1128 – same keyframeMax clamp
ratecontrol.cpp:1888 – m_param->totalFrames <= 2 * m_fps
ratecontrol.cpp:1935/1937 – curBitrate/projectedBitrate AQ calc
ratecontrol.cpp:2332 – m_framesDone < (int)(m_fps + 0.5)
ratecontrol.cpp:3518/3525 – forwardMasking() scenecut windows

If I'm reading it right, m_fps is never reassigned anywhere else, so those reads are of an uninitialized member. That would explain the pattern you're seeing pretty well — MSVC's debug CRT poisons freshly allocated memory, so an uninitialized double there would come back with garbage in Debug but often "accidentally work" in Release. And since m_fps is only touched in the ABR/VBV/CBR paths, it'd only bite --bitrate runs, not CRF/CQP ones.

Could well be I'm missing an initializer somewhere else in the flow — if so, apologies for the noise! But if it checks out, restoring something like:

cpp
m_fps = (double)m_param->fpsNum / m_param->fpsDenom; // or 1.0 / m_timebase

right next to the new m_timebase line seems like it should clear it up.

[cubicibo]

Thank you, a bad oversight of mine. Unfortunately that does not solve it.

[mcw-Lavanya]

Thank you, a bad oversight of mine. Unfortunately that does not solve it.

0001-ratecontrol-initialize-m_durationDone.patch

Hi @cubicibo ,
kindly apply this patch on this PR and let us know if it is not affecting the PR behaviour and at the same time fixing the smoke test output issue

[mcw-Lavanya]

Hi @cubicibo ,
The build failures are due to some warnings present in code previously, that is fixed from our end and raised a PR, once that PR is merged I shall intimate you , you can commit empty changes to trigger the Ci pipeline again for the CI tests to pass.

Thank you

[mcw-Lavanya]

Hi @cubicibo,
Kindly rebase the PR with current master, a warning is being fixed in master commit.
Thank you

[mcw-Lavanya]

Hi @cubicibo ,
Can we get the test setup, CLI and benchmarking results of this PR to verify from our end?

Thank you

[cubicibo]

I have found one more conformance issue. However it is present in the master branch, it does not come from this PR. x265 bitstreams sometime breach Equation C-18 (§ C.4). This is not a problem for most decoders using the H.222 leak method... but it still needs fixing. I will try to address it in another PR first.

Regarding your question:

Test set-up & toolings

• Python HEVC HRD Verifier: implements all CPB equations of section H.265 C.x, and DPB output timing.
• MT8581-based STB

I have included a readme to the Python HRD verifier. You are free to make any use of it.

Command lines

1. --psfile: Specify pulldown information through file.

• psf.txt to encode 4 framerates section: 23.976, 29.97, 59.94 and 47.95 in 59.94 HEVC container.
• Test purpose: verify --keyint 60 is indeed at most 60 clock ticks apart, regardless of the section's framerate.

./x265 --y4m - -o ~/output.265 --vbv-maxrate 10000 --vbv-bufsize 8000 --fps 60000/1001 --psfile ~/psf.txt --hrd --aud --repeat-headers --keyint 60 --bitrate 6500

2. --prepulldown-fps prepulldown.patch (not commited yet) : This lets you specify the input framerate, prior to pulldown to --fps.

• Purpose: offer more versatile equivalent to x264 --pulldown argument.
• Useful for broadcast, where movies are 24p and must be delivered via 50p or 60p.

./x265 --y4m - -o ~/output.265 --vbv-maxrate 10000 --vbv-bufsize 8000 --fps 60000/1001 --prepulldown-fps 24000/1001 --hrd --aud --repeat-headers --keyint 60 --bitrate 6500

Evaluation

1. A STB plays the bitstream at the appropriate framerate after muxing to a Transport Stream thanks to picture-structure markings.
2. You can evaluate the DPB output timing, and CPB usage with the provided Python HRD verifier.

python3 client.py --dpb timing.txt --plot vbv.png --logfile hss.txt output.265

With command line 2) on an arbitrary video:
Example DPB timing output - As you can see, every picture is alternatively doubled and tripled to attain 59.94 fps
Example VBV plot

[mcw-Lavanya]

I have found one more conformance issue. However it is present in the master branch, it does not come from this PR. x265 bitstreams sometime breach Equation C-18 (§ C.4). This is not a problem for most decoders using the H.222 leak method... but it still needs fixing. I will try to address it in another PR first.

Regarding your question:

Test set-up & toolings

• Python HEVC HRD Verifier: implements all CPB equations of section H.265 C.x, and DPB output timing.
• MT8581-based STB

I have included a readme to the Python HRD verifier. You are free to make any use of it.

Command lines

1. --psfile: Specify pulldown information through file.

• psf.txt to encode 4 framerates section: 23.976, 29.97, 59.94 and 47.95 in 59.94 HEVC container.
• Test purpose: verify --keyint 60 is indeed at most 60 clock ticks apart, regardless of the section's framerate.

./x265 --y4m - -o ~/output.265 --vbv-maxrate 10000 --vbv-bufsize 8000 --fps 60000/1001 --psfile ~/psf.txt --hrd --aud --repeat-headers --keyint 60 --bitrate 6500

2. --prepulldown-fps prepulldown.patch (not commited yet) : This lets you specify the input framerate, prior to pulldown to --fps.

• Purpose: offer more versatile equivalent to x264 --pulldown argument.
• Useful for broadcast, where movies are 24p and must be delivered via 50p or 60p.

./x265 --y4m - -o ~/output.265 --vbv-maxrate 10000 --vbv-bufsize 8000 --fps 60000/1001 --prepulldown-fps 24000/1001 --hrd --aud --repeat-headers --keyint 60 --bitrate 6500

Evaluation

1. A STB plays the bitstream at the appropriate framerate after muxing to a Transport Stream thanks to picture-structure markings.
2. You can evaluate the DPB output timing, and CPB usage with the provided Python HRD verifier.

python3 client.py --dpb timing.txt --plot vbv.png --logfile hss.txt output.265

With command line 2) on an arbitrary video: Example DPB timing output - As you can see, every picture is alternatively doubled and tripled to attain 59.94 fps Example VBV plot

Thank you @cubicibo , we will verify from our end and get back to you

[mcw-Lavanya]

Hi @cubicibo ,
Kindly update the help page of x265 for using --psfile in cli which is added in cli.rst.
code LGTM

[mcw-Lavanya]

Hi @cubicibo ,
May I know why this line of code is repeated

[mcw-Lavanya]

Hi @cubicibo ,
Is it possible to create the prepulldown related commits in new PR so that it would be easier for understanding and reference,
psfile related commits we can push separately,
Let us know if it is possible, or we shall review the new code here
Thank you

[cubicibo]

Apologies, I will revert that commit. I had figured it was an easier entry-point to most users contrary to the --ps-file.

[mcw-Lavanya]

Hi @cubicibo,

Thank you for the updated PR and all the work on it. While testing, I ran into a segmentation fault with temporal layers and wanted to share it in case it helps.

Command:

x265 --input BasketballDrive_1920x1080_50.y4m --vbv-bufsize 100000 --vbv-maxrate 90000 --fps 50 --keyint 25 --temporal-layers 5 --frames 60 -o out.hevc

What I saw:

master : encodes all 60 frames without problems.
PR (e552e3b): segmentation fault around frame 24.
It also crashes without --hrd and without --pic-struct, so it doesn’t seem to depend on those options.

Where I think the problem might be (I may be wrong):
In Lookahead::slicetypeDecide, when temporal layers are above 2 and a mini-GOP is partial, the split path seems to declare int idx = 0; again for each sub-GOP and again in the leftOver < 4 block. If I’m reading it right, entries in codedFrameOrderedIndex[] could get overwritten, and some might be left uninitialised. The new loop

for (int i = 0; i <= bframes; ++i)
calculateDurations(list[codedFrameOrderedIndex[i]], prevFrame);

might then read an invalid index. With --keyint 25 and 15 B-frames, the GOP ends in a partial mini-GOP around frame 24, which seems to match where it crashes.

If that’s the cause, keeping one running counter for codedFrameOrderedIndex across all sub-GOPs (separate from the per-sub-GOP pts index), and looping calculateDurations over that count, might fix it. I may well be missing something, so please correct me if this isn’t it.

The existing regression test --temporal-layers 5 --b-adapt 0 --keyint 50 may also hit this path.

Thank you!

[cubicibo]

Thank you for the report. It should be fixed with the last push with cleaned-up commit history, in e03c6b2

[mcw-Lavanya]

Hi @cubicibo,

With --psfile, a pic struct value keeps applying to every following frame until the next line in the file. For example, with:

0 0 0
10 0 7
20 0 8
21 0 7
22 0 8
23 0 7
40 0 0

the single 23 0 7 line makes frames 23–39 all use pic_struct 7. We confirmed this with trace_headers: 28 frames carry 7, although only frames 10, 21 and 23 list it.

Could you confirm whether this is intended?
It looks like it may not be:
The comment at the end of parsePSFile() says “not changed, use frame’s initial value (default)”. That suggests frames with no entry were meant to fall back to the default, not inherit the previous value.
In practice the value carries over because the CLI reuses the same x265_picture (pic_orig) for every frame and only sets picStruct when there’s a matching line. So this happens as a side effect of reusing the picture, not because the parser explicitly carries values forward.

If it is intended, i.e. each line sets the structure “from this frame onwards”, which is convenient for pulldown sections:
Please document it in cli.rst and in the --psfile help text. For example: “A picture structure applies from the given frame until the next entry. Use 0 to return to progressive.”
Please fix the comment in parsePSFile() to say the previous value carries over.

If it is not intended, i.e. each line applies only to that one frame:
Please reset picStruct to PIC_STRUCT_AUTO for frames that have no entry. For example, set pic_org.picStruct = PIC_STRUCT_AUTO at the start of parsePSFile(), or before each frame in abrEncApp.cpp.
Either way, please describe the behaviour in the docs, so that users writing psfiles for VFR or pulldown know whether they must list every frame or only the frames where the structure changes.

With --temporal-layers 3 --hrd, the global --pic-struct 7 is correctly ignored (pic_struct is all 0, with a warning).
With --psfile, there is no warning, and the bitstream carries pic_struct 7 and 8 (28 × 7, 2 × 8 in a 60-frame encode).

2. Unsigned underflow in tuneAbrQScaleFromFeedback

m_durationDone - frameNumThreads + 1 is computed in uint64_t, so for the first few frames it wraps to about 1.8e19 instead of going negative.
That can halve the ABR qScale on early frames, depending on thread timing. Master used an int here, so the result went negative and the adjustment was skipped.
Fix: (double)((int64_t)m_durationDone - m_param->frameNumThreads + 1) * m_timebase.

3. --intra-refresh --hrd: CPB removal delay is never reset (a regression)

x265 --input BasketballDrive_1920x1080_50.y4m --intra-refresh --hrd --vbv-bufsize 2000 --vbv-maxrate 1500 --bitrate 1200 --keyint 30 --frames 150

au_cpb_removal_delay_minus1 from trace_headers, one line per Buffering Period:

	Master	PR
BP 1	0 0 1 2 … 25	0 0 1 2 … 25
BP 2	26 0 1 2 … 31	26 27 28 … 58
BP 3	32 0 1 2 … 29	59 60 61 … 89
BP 4	30 0 1 2 … 30	90 91 92 … 121
BP 5	31 0 1 2 … 25	122 123 … 148

The BP positions are identical, but the PR never restarts the delay. With au_cpb_removal_delay_length_minus1 = 9, the field saturates after about 1024 ticks. With --pic-struct 8 it climbs from 0 to 445 over the same 5 BPs.

Lookahead::calculateDurations() only resets m_cpbDelay when !bIntraRefresh && bKeyframe. With intra refresh, the keyframe and its BP SEI are decided later, in Encoder (PIR logic, ~encoder.cpp:1095).

Suggested fix: compute the delay relative to the last BP at the point where the BP SEI is written, as master did with m_lastBPSEI. For example, keep a cumulative tick count in the lookahead and subtract the value recorded at the last BP in FrameEncoder.

Related: the intra-refresh period still counts frames (framesSinceLastPir >= keyframeMax), while --keyint now counts ticks. With --pic-struct 8 --keyint 30, BPs come every ~30 frames (90 ticks), while normal keyframes come every 10 frames. Please make them consistent, or document the difference.

4. User pic struct is applied to interlaced field pictures

With --interlace tff --hrd --pic-struct 7, every field is signalled as pic_struct 7, and there is no warning.
That isn’t allowed: field pictures (field_seq_flag = 1) may only use pic_struct 1, 2 or 9–12. Master always derived 1/2 itself.
Rate control also treats each field as 2 ticks long, so the bitrate and QP change.

Could you please check the points above and let us know your thoughts? Thank you for your quick responses on the earlier fixes.

Thank you

[cubicibo]

Thank you very much for the exhaustive testing. I have addressed all of the above.

With --psfile, a pic struct value keeps applying to every following frame until the next line in the file. For example, with:

I went with a reset to PIC_STRUCT_AUTO for every input pic without an entry in the psfile. Carrying the structure would cause orphaned fields with interlaced content.

The encoder aborts whenever psfile + temporal layers or interlacing are used together. Interlace + psfile support will come in a separate PR.

--intra-refresh --hrd: CPB removal delay is never reset (a regression)

Fixed, it had to be done in encoder.cpp, not in frameencoder because those are threaded. I made PIR operate on the tick count, not frame count as suggested. I also tested with intra-refresh + qpfile with forced keyframes and auCpbRemovalDelayMinus1 looked correct.

User pic struct is applied to interlaced field pictures && Unsigned underflow in tuneAbrQScaleFromFeedback

Prevented & fixed. Only element missing is preventing structure 3,4,5,6 on field sequences. I will push that once this pipeline succeed.

[mcw-Lavanya]

Hi @cubicibo,

Thank you for the latest update (4d16988). The interlaced pic-struct handling and the --psfile / --pic-struct conflict check look good now, and CI is green.

We have a few final points to clear before we can approve:

1. Encodes with alpha or multiview layers get the wrong timing

In Encoder::encode(), m_clockTickCount += inFrame[layer]->m_duration; is inside the for (int layer = 0; layer < m_param->numLayers; layer++) loop. With two layers (for example --alpha, which sets numLayers = 2), the clock moves forward twice per access unit:

• Base-layer frames get m_displayPicCount 0, 2, 4, …, while the lookahead's m_codedPicCount moves forward by 1 per frame.
• Because keyint now counts ticks, --keyint 60 puts a keyframe every 30 frames.
• With --hrd, pic_dpb_output_delay (m_displayPicCount - m_codedPicCount + reorder) keeps growing for the whole encode.

None of the CI jobs builds with ENABLE_ALPHA / ENABLE_MULTIVIEW, so CI can't catch this.

Suggested fix: give every layer the same timestamp and advance the clock once per access unit, for example after the layer loop:

inFrame[layer]->m_duration = g_deltaToDivisor[inFrame[layer]->m_picStruct];
inFrame[layer]->m_displayPicCount = m_clockTickCount;
...
} // end of layer loop
m_clockTickCount += inFrame[0]->m_duration;

2. --psfile without --hrd misses the target bitrate with no warning

--pic-struct is disabled in configure() when --hrd is off, but --psfile isn't checked in the same way:

• Frames marked 7 or 8 in the psfile get a duration of 2 or 3 ticks in rate control, keyint and statistics.
• Without --hrd, no Picture Timing SEI is written and frame_field_info_present_flag is 0, so a decoder plays every frame as 1 tick.
• The actual bitrate can be up to ~3× the target, and the VBV constraints don't hold for the real stream.

The same thing happens for libx265 API users who set x265_picture::picStruct without HRD.

Suggested fix:

• In CLIOptions::checkParamsWithPSFile():

  if (!globalParam->bEmitHRDSEI)
  {
      x265_log(param, X265_LOG_ERROR, "psfile requires --hrd.\n");
      return true;
  }
  

• In Encoder::encode(), ignore the per-picture structure when HRD is off, to cover API callers:

  if (inFrame[layer]->m_param->pictureStructure >= PIC_STRUCT_AUTO)
      inFrame[layer]->m_picStruct = inFrame[layer]->m_param->pictureStructure;
  else if (inFrame[layer]->m_param->bEmitHRDSEI)
      inFrame[layer]->m_picStruct = inputPic[0]->picStruct;
  else
      inFrame[layer]->m_picStruct = PIC_STRUCT_PROGRESSIVE_FRAME;
  

3. Documentation

• cli.rst: --keyint still says "Max intra period in frames". It now counts clock ticks, which differ from frames when --pic-struct, --psfile or --frame-dup is used. Please update --keyint and --min-keyint, including how this affects --intra-refresh.
• cli.rst and the CLI help: please state that --pic-struct and --psfile need --hrd. Otherwise they are ignored or rejected.
• api.rst: please document the new public PicStruct enum in x265.h, and that x265_picture::picStruct is now honoured per picture (with HRD enabled).

Once these are in, please rebase on the current master (the branch is 3 commits behind) so CI runs on the final head. We'll approve after that.

Thank you!

[mcw-Lavanya]

Thank you for the updated code, LGTM