Disabled NAL HRD parameters submission for now. picture_timing() SEI messages would also be necessary and more careful generation of headers is needed.

How about keeping pic_struct_present_flag = FALSE ? so that pic_timing SEI header will limit to cpb_removal_delay and  dpb_removal_delay fields. Because we have to generate pic_timing() sei for all frames, adding the extra pic_timing SEI fields ( = fields when pic_struct_present_flag is TRUE) doesn't provide much advantage IMHO.
The x264 encoder is also not setting those values it seems.

I think the CBR mode is completely broken in current gstreamer-vaapi.
Because if we enable CBR mode , the driver will insert the SEI headers by itself. But we have disabled the nal_hrd_parameters_present_flag in gstreamer-vaapi h264 encoder. So while we try to play the encoded video with gstreamer-vaapi-decoder, it will fail to parse sei headers since it is based on nal_hrd_parameters_present_flag. 

If we set the nal_hrd_parameters_present_flag to TRUE with in encoder, then the decoding of corresponding encoded stream will work, but there are many distortions in the video. Especially if we allow bitrate auto calculation.

There are many things which we need to track related with this:

-- CBR mode is not at all working correctly for me. 

-- I haven't seen an implementation in driver to handle the VAEncMiscParameterRateControl, may be this is only needed for VBR mode

-- h264codecparser need a fix which is tracked in #734970

--MVC encoding needs the mvc_vui_parameters_extension() which is not yet implemented (with in subset_sequence_packed_header).

Anyway I will provide the patch for sending buffering_period and pic_timing SEI messages for some review.

Created attachment 283695 [details] [review]
 encoder: h264: submit SEI buffering_period() and picture_timing() messages

Aha, There are two unused variable declarations in the patch :(. I will remove those after getting other reviews.

Review of attachment 283695 [details] [review]:

The rest looks correct.

::: gst-libs/gst/vaapi/gstvaapiencoder_h264.c
@@ +118,3 @@
   guint cur_frame_num;
   guint cur_present_index;
+  guint frame_count; /* monotonically increasing with in every idr period */

Move up to after frame_index? For alignment and other cosmetical purposes. :)

@@ +837,3 @@
+   * but adding a tolerance of one frame duration,
+   * which is 2 more clock-ticks */
+  cpb_removal_delay = (reorder_pool->frame_count * 2 + 2);

I have not checked the standard, but does this still hold true even for progressive sequences?

(In reply to comment #7)
> Review of attachment 283695 [details] [review]:
> 
> The rest looks correct.
> 
> ::: gst-libs/gst/vaapi/gstvaapiencoder_h264.c
> @@ +118,3 @@
>    guint cur_frame_num;
>    guint cur_present_index;
> +  guint frame_count; /* monotonically increasing with in every idr period */
> 
> Move up to after frame_index? For alignment and other cosmetical purposes. :)

k :)

> @@ +837,3 @@
> +   * but adding a tolerance of one frame duration,
> +   * which is 2 more clock-ticks */
> +  cpb_removal_delay = (reorder_pool->frame_count * 2 + 2);
> 
> I have not checked the standard, but does this still hold true even for
> progressive sequences?

I am bit confused with the exact values. Does the spec mention any specific values ??.This calculations(tolerance of one frame duration) were based on some papers I have read here and there.

Created attachment 291694 [details] [review]
encoder: h264: submit SEI buffering_period() and picture_timing() messages

I would like you get a final confirmation regarding this before pushing :)
Let's move it to 0.5.11..

Pushed,
commit a2604261946e0716dd2708686454d3ad6262f938
Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
Date:   Thu Jul 2 21:00:14 2015 +0300

    encoder: h264: submit SEI buffering_period() and picture_timing() messages for CBR mode