Re: [f265 dev team] VPS timing info patch

[Laurent Birtz]

Here is the commented review.

Regards,
Laurent

Please search for %%%.

diff --git a/f265/bs.c b/f265/bs.c
index f6bde52..57c47af 100644
--- a/f265/bs.c
+++ b/f265/bs.c
@@ -183,7 +183,18 @@ void fenc_write_vps(f265_vlc_bs *vbs, f265_enc *enc)
     FENC_PUT_UE_V(vbs, 0, "vps_max_latency_increase_plus1");
     FENC_PUT_BITS(vbs, 0, 6, "vps_max_nuh_reserved_zero_layer_id");
     FENC_PUT_UE_V(vbs, 0, "vps_max_op_sets_minus1");
-    FENC_PUT_FLAG(vbs, 0, "vps_timing_info_present_flag");
+
+    int timing_flag = gd->vps_timing_flag;
+    FENC_PUT_FLAG(vbs, timing_flag, "vps_timing_info_present_flag");
+    if (timing_flag)
+    {
+        FENC_PUT_BITS(vbs, gd->frame_rate_den, 32, "vps_num_units_in_tick");
+        FENC_PUT_BITS(vbs, gd->frame_rate_num, 32, "vps_time_scale");
+        FENC_PUT_FLAG(vbs, 1, "vps_poc_proportional_to_timing_flag");
+        FENC_PUT_UE_V(vbs, 0, "vps_num_ticks_poc_diff_one_minus1");
+        FENC_PUT_UE_V(vbs, 0, "vps_num_hrd_parameters");
+    }
+
     FENC_PUT_FLAG(vbs, 0, "vps_extension_flag");
     fenc_vlc_flush(vbs);
 }
diff --git a/f265/enc.c b/f265/enc.c
index d2d208b..188dc3c 100644
--- a/f265/enc.c
+++ b/f265/enc.c
@@ -679,6 +679,7 @@ static void fenc_init_enc_mem(f265_enc_params *p, f265_enc_mem_data *d, char **e
     gd->nb_reordered_frames = p->nb_b_frames ? p->nb_b_refs + 1 : 0;
     gd->frame_rate_num = p->frame_rate_num;
     gd->frame_rate_den = p->frame_rate_den;
%%% Normalization must be done in f265_normalize_params() in bdi.c.
%%% Placement is OK, just after frame_rate_den.
+    gd->vps_timing_flag = !!p->vps_timing_flag;
     gd->algo = p->algo;
 
     f265_me_search_func sf[4] = {fenc_me_dia_search, fenc_me_xdia_search, fenc_me_hex_search, fenc_me_square_search};
diff --git a/f265/enc.h b/f265/enc.h
index bca2e55..fb651b7 100644
--- a/f265/enc.h
+++ b/f265/enc.h
@@ -2274,6 +2274,9 @@ typedef struct f265_gen_data
     // Tile layout.
     int8_t tiles[2];
 
%%% Please keep the ordering you defined above for all operations.
%%% It's easier to verify if things are initialized properly.
%%% Should be below
%%% int32_t frame_rate_num;
%%% int32_t frame_rate_den;
+    // True if VPS timing info is to be written.
+    int8_t vps_timing_flag;
+
     // == Direct parameter import end ==
 
     // Initial luma QP.
diff --git a/f265/f265.h b/f265/f265.h
index d1d0a2e..e5f787b 100644
--- a/f265/f265.h
+++ b/f265/f265.h
@@ -215,6 +215,9 @@ typedef struct f265_enc_params
     // True if the frame bitstream can be reformatted multiple times.
     int8_t reformat_flag;
 
%%% Same comment as above.
+    // True if VPS timing info is to be written.
+    int8_t vps_timing_flag;
+
 
     // Lookahead.
 
diff --git a/f265/parse.c b/f265/parse.c
index ab4cd0a..08426e3 100644
--- a/f265/parse.c
+++ b/f265/parse.c
@@ -499,6 +499,11 @@ static void handle_param_nb_workers(f265_parse_ctx *ctx, f265_enc_params *p, f26
     p->nb_workers[1] = a[1].i;
 }
 
 %%% Same, should be after "fps" parameter.
+static void handle_param_vps_timing(f265_parse_ctx *ctx, f265_enc_params *p, f265_parse_arg *a, int32_t nb_args)
+{
+    p->vps_timing_flag = a[0].i;
+}
+
 // Parameter dispatch table.
 static const f265_parse_entry f265_enc_params_table[] =
 {
@@ -554,6 +559,7 @@ static const f265_parse_entry f265_enc_params_table[] =
     { "tiles", handle_param_tiles, 2, 0 },
     { "mt-mode", handle_param_mt_mode, 1, 0 },
     { "nb-workers", handle_param_nb_workers, 2, 0 },
%%% Same, should be after "fps" parameter.
+    { "vps-timing", handle_param_vps_timing, 1, 0 },
 };
 
 void f265_parse_params(f265_enc_params *params, const char *param_string, char **error_handle)

%%% Document the paramater in params.txt.