va: Implement vah264enc (!1051) · Merge requests · GStreamer / gstreamer · GitLa...

Nicolas Dufresne - @ndufresne wrote here on 07, Sep 2021:

This is just one random comment, didn't took the time to review yet. This bit-writer seems to return a freshly allocated byte buffer, which defeats our attempt to avoid run-time allocation. Can we think of a way to write this into a pre-allocated and pooled block ? Of course that makes the API a tad more complex, in the case the block was too small, but I think it would be a nice feature.

Víctor Manuel Jáquez Leal - @vjaquez wrote here on 07, Sep 2021:

Perhaps its only me, but I would avoid a base class for now, to avoid premature optimizations. All in a single element, and later decouple as needed.

He Junyan - @He_Junyan wrote here on 07, Sep 2021:

OK. I just want the base class to handle all misc thing, such as caps setting and multi-threading sync, and let the sub class implement

  gboolean (*reconfigure)    (GstVaBaseEnc * encoder);
  gboolean (*push_frame)     (GstVaBaseEnc * encoder,
                              GstVaEncFrame * frame,
                              gboolean last);
  gboolean (*pop_frame)      (GstVaBaseEnc * encoder,
                              GstVaEncFrame ** out_frame);
  gboolean (*encode_frame)   (GstVaBaseEnc * encoder,
                              GstVaEncFrame * frame);

to handle the real encoding.

It OK to mix them all as one for the first encoder, and split them when we write the second one.

Nicolas Dufresne - @ndufresne wrote here on 07, Sep 2021:

When I saw "baseclass", I thought of a proper libgstcodecs base class. I would rather consider that stateless encoder will use a base class from libgstcodecs and experiement with that.

Even though simpler, we still need to have something like a DPB, and we might need some tree structure for more advance coding patterns. Bit writers are helper seems fair though, but they could be in the per codec base classes to reduce the boiler plate code.

@sree also would like us to support software bitrate control, even for VA. His lib will likely never provide rate control for anything other then VP9 and AV1 though, so we'd probably also need some in-gstreamer "fallback" implementation, so it minimally works. Of course, most VA driver have their own, as VA API allow that.

He Junyan - @He_Junyan wrote here on 08, Sep 2021:

I think the problem in current stage is that we do not know what is the common logic for all encoders. Different encoders have different preference and limitation. And I agree with @vjaquez , firstly, write a lean and logic clean encoder, and move the common logic to some base iterative if we find.

About the rate control, I think we can provide a lib to analyse the relationship between the buffer fullness and the incoming frames, decide its init QP and other parameters. We can even write some lookahead element to do the better analysis, and link it before the encoder.

These are big pictures and it needs a lot of effort:). I firstly need to implement a VA encoder in good code logic and with median compression quality(Just depending on HW rate control).

Nicolas Dufresne - @ndufresne wrote here on 08, Sep 2021:

Well, remember that we found impossible to extract the logic from the VA code in GStreamer VA-API.

He Junyan - @He_Junyan wrote here on 09, Sep 2021:

added 1 commit

• 24b59434 - Add the encoder and va base encoder for h264

Compare with previous version

He Junyan - @He_Junyan wrote here on 14, Sep 2021:

added 30 commits

• 24b59434...364a2fe0 - 28 commits from branch gstreamer:master
• 461cfe6c - Add a generic bit code writer for H264.
• 785b8226 - Add the encoder and va base encoder for h264

Compare with previous version

Víctor Manuel Jáquez Leal - @vjaquez wrote here on 16, Sep 2021:

btw, my original idea is to have low power entrypoint as different elements. So in the end will have vah264enc and vah264lpenc and the different devices, vaD129h264lpenc

In that way, iiuc, the logic will be simpler and the user will be in control of what they want.

Still the source will be the same for all of them, just a bool in the class structure.

Víctor Manuel Jáquez Leal - @vjaquez wrote here on 16, Sep 2021:

If I understand correctly many of these properties are driver-dependent, so they should be autogenerated as GstVaFilter does for vapostproc and vadeinterlace.

U. Artie Eoff - @ullysses.a.eoff wrote here on 16, Sep 2021:

It would be nice if we could synergize the property names between the various hw plugin elements (e.g. va, vaapi, and msdk plugins).

Currently:

• b-frames is used here and msdkh264enc but max-bframes is used in vaapih264enc.
• keyframe-period is used here and vaapih264enc but gop-size is used in msdkh264enc.
• ref-frames is used here and msdkh264enc but refs is used in vaapih264enc.

... there are other properties that have different names but the same function, too.

If we used the same property names across plugins, then it would make it a whole lot easier for users to remember. And way easier to write boilerplate scripts for testing the various plugins. At minimum, can we synergize property names between va and msdk?

He Junyan - @He_Junyan wrote here on 18, Sep 2021:

mentioned in merge request !886 (merged)

He Junyan - @He_Junyan wrote here on 04, Oct 2021:

added 57 commits

• 785b8226...ca8068c6 - 49 commits from branch gstreamer:master
• 9144f3f1 - codecparsers: nalutils: Add nal_writer_reset_and_get_data help function.
• aaaa458c - codecparsers: bitwriter: Add the common bit writer functions for H264.
• d56a817a - test: Add test cases for the H264 bitwriter.
• c4f165fa - va: Make the gst_va_create_coded_caps as a common helper function.
• 2ac07697 - va: encoder: Add a common encoder object.
• 9e1cbf06 - va: encoder: Import the H264 encoder.
• 26807ae8 - va: enable the H264 encoder.
• a0bfff0f - va: Change the H264 profile string order in the profile_map.

Compare with previous version

He Junyan - @He_Junyan wrote here on 04, Oct 2021:

marked this merge request as ready

He Junyan - @He_Junyan wrote here on 04, Oct 2021:

@vjaquez I am a little sorry, I make that H264 encoder not so simple and added most of the properties in vaapi. Because I need to measure the performance of it and compare the BD-rate with other encoders:)

He Junyan - @He_Junyan wrote here on 04, Oct 2021:

I will try to do it, thanks for your notify.

He Junyan - @He_Junyan wrote here on 04, Oct 2021:

mentioned in merge request !1047 (closed)

[91/1018] Compiling C object subprojects/gst-plugins-bad/gst-libs/gst/codecparsers/libgstcodecparsers-1.0.so.0.1902.0.p/gsth264bitwriter.c.o                                                                       
../subprojects/gst-plugins-bad/gst-libs/gst/codecparsers/gsth264bitwriter.c:570:3: warning: result of comparison of constant -12 with expression of type 'const guint8' (aka 'const unsigned char') is always false
 [-Wtautological-constant-out-of-range-compare]                                                                                                                                                                    
  WRITE_SE_RANGE (bw, pps->second_chroma_qp_index_offset, -12, 12);                                                                                                                                                
  ^                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~                                                                                                                                                      
../subprojects/gst-plugins-bad/gst-libs/gst/codecparsers/gsth264bitwriter.c:84:24: note: expanded from macro 'WRITE_SE_RANGE'                                                                                      
  if (val > max || val < min ||                                               \                                                                                                                                    
                   ~~~ ^ ~~~                                                                                                                                                                                       
1 warning generated.                  

fwiw, I just started reading Direct3D12 video encoding APIs (part of Windows 11 SDK) and got an impression that overall flow is very similar to that of VA encoding