Extend parameter buffers to support Hantro G1 H.264 #332

pH5 · 2019-09-05T13:46:45Z

The Hantro G1 hardware H.264 decoder parses slices itself, with some assistance.
It requires the parsed num_ref_idx_l[01]_default_active_minus1 and idr_pic_id syntax elements, as well as the size in bits of the dec_ref_pic_marking() syntax element and the total size in bits of the pic_order_cnt_lsb/delta_pic_order_cnt_bottom/delta_pic_order_cnt[0]/delta_pic_order_cnt[1] syntax elements.

These patches extend VAPictureParameterBufferH264 and VASliceParameterBufferH264 to allow supporting Hantro G1 decoders via libva-v4l2-request.

…erH264 The Hantro G1 hardware H.264 decoder requires this information. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

The Hantro G1 hardware H.264 decoder requires this information. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

…eterBufferH264 Add the size in bits of the pic_order_cnt_lsb, delta_pic_order_cnt_bottom, delta_pic_order_cnt[0], and delta_pic_order_cnt[1] syntax elements. The Hantro G1 hardware H.264 decoder requires this information. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

…ureParameterBufferH264 The Hantro G1 hardware H.264 decoder requires this information. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

XinfengZhang · 2019-09-05T15:32:31Z

va/va.h

+    uint8_t num_ref_idx_l1_default_active_minus1;

    /** \brief Reserved bytes for future use, must be zero */
    uint32_t                va_reserved[VA_PADDING_MEDIUM];


I understand that you add these field between the crevice of alignment, have you checked the size both on 64bit and 32 bit?

Yes, there is no difference between 64-bit (x86_64) and 32-bit (arm-v7a): In both cases, sizeof(VAPictureParameterBufferH264) is 672 bytes before and after the change. All member offsets stay the same.

sizeof(VASliceParameterBufferH264) is 3128 bytes before and after the change on both 64-bit and 32-bit. The only member offset that changes is va_reserved, which moves from 3112 to 3120.

Shouldn't va_reseverd size be reduced to maintain overall structure size ?

Overall structure size is maintained by placing the new elements inside previously unused 32-bit alignment padding, see the comment above.

Bump the API version to 1.6.1 due to the fields added to VAPictureParameterBufferH264 and VASliceParameterBufferH264.

CameronNemo · 2020-10-05T02:20:05Z

Any word on whether this will be reviewed and/or merged?

ndufresne · 2020-10-13T14:17:46Z

Any word on whether this will be reviewed and/or merged?

With native support for V4L2 stateless codecs making it into GStreamer, and being present in Chrome with pending patches for ffmpeg, support for Hantro through VA API isn't a priority to me any-more. Our feeling is that VA as a way to regroup vendor under the same API is not longer an Intel priority (up to Intel to correct here). So as long as it fits AMD, I suspect patches like this one will remain ignored forever.

CameronNemo · 2020-10-13T19:00:36Z

@ndufresne thanks for the update. unfortunately firefox seems to depend on vaapi and cannot use v4l2 codecs directly. are there any non-libva options for firefox video acceleration?

okias · 2020-10-13T19:02:01Z

@ndufresne thanks for the update. unfortunately firefox seems to depend on vaapi and cannot use v4l2 codecs directly. are there any non-libva options for firefox video acceleration?

sadly, Firefox is in bad shape, so I assume Red Hat guys won't be contributing with another backend.

ndufresne · 2020-10-13T19:42:21Z

Then feel free to carry on, you'll have to take the V4L2 Request VA driver maintenance though, as it no longer has a maintainer.

fhvwy · 2020-02-10T22:11:20Z

va/va.h

    int8_t slice_alpha_c0_offset_div2;
    int8_t slice_beta_offset_div2;
+    /** \brief Same as the H.264 bitstream syntax element. */
+    uint16_t  idr_pic_id;


Why does a decoder want to know this number? It doesn't affect decoding at all - it's only used for AU splitting.

(If you need a made-up number to give to an external interface which requires it for some reason then passing an alternating sequence of zeroes and ones will always suffice.)

fhvwy · 2020-02-10T22:16:05Z

va/va.h

+    /** \brief Same as the H.264 bitstream syntax element. */
+    uint8_t num_ref_idx_l0_default_active_minus1;
+    /** \brief Same as the H.264 bitstream syntax element. */
+    uint8_t num_ref_idx_l1_default_active_minus1;


These values are meaningless for a decoder - it should only be looking at the values for num_ref_idx_lN_active_minus1, which are provided at the slice level.

(Copying the slice numbers will necessarily be sufficient for any purpose, since a new PPS with those values could always have been present in the stream immediately before the access unit you are looking at.)

Hantro/VSI hardware works at frame level. For my own understanding, how do one derive from slices this information ?

from syntax level, num_ref_idx_lN_active_minus1 will be override by slice parameter (depend on whether num_ref_idx_active_override_flag is true). so I dont think that you just need a frame level syntax for it. if there are multiple slice with different values. how to handle it?

Ok, I'm not VSI employee, so I can only guess thing, let me know if that make sense. The HW will be parsing the value from the bitstream by itself, ignoring the values we have in VA-API structure per slice. Now, if it encounter a slice that does not have this syntax, it needs to use the default value, which we are adding here.

My question is, can we decuce from the information already in VA-API what is the default value ? Is it the maximum of all slices or something like this ?

if application parse the bitstream, it always know whether the default value should be overridden. it's why we just use slice parameter to set the value no matter it use default value or value from slice. it could cover all of them.

so, I assume your problem is: application just parse part of the bitstream, SPS, PPS. then send other remaining bitstream to HW. HW may handle the sliceheader parser, and check whether it should be overridden. right?

for this case, the easiest way is sending a slice parameter, which just use to send these two parameters . other parameters in the slice parameter will be ignored.

Said like this is sounds more like a hack, the application have no idea if the internal is slice or frame based. If you set the default in the firs slice, and the first slice wasn't using the defaults, you endup with the wrong value. I don't really know what this one is for and the semantic of it internally, that's a bit why I ask what to with only the squashed version (per slice final result). I think these two are somewhat legit request. But I guess it does not matter, since the driver would need reparse anyway to figure-out the skip bits parameters. I think the double parse overhead is unavoidable in the context of VA-API.with this type of HW.

fhvwy · 2020-02-10T22:22:13Z

va/va.h

    int16_t chroma_weight_l1[32][2];
    int16_t chroma_offset_l1[32][2];
+    /** \brief Size in bits of the dec_ref_pic_marking() syntax element. */
+    uint32_t dec_ref_pic_marking_bit_size;


What are you going to do with this number? The semantics of dec_ref_pic_marking() are entirely handled by the caller in their reference frame management.

The Hantro/VSI hardware do not have code to parse this, instead the size of that part is passed so the HW can simply skip it. This is hardware specific.

fhvwy · 2020-02-10T22:25:46Z

va/va.h

+     * Size in bits of the pic_order_cnt_lsb, delta_pic_order_cnt_bottom,
+     * delta_pic_order_cnt[0], and delta_pic_order_cnt[1] syntax elements.
+     */
+    uint32_t pic_order_cnt_bit_size;


Why do you need this? The TopFieldOrderCnt / BottomFieldOrderCnt values resulting from the caller processing these fields are already supplied in VAPictureParameterBufferH264.CurrPic.

Same, arguably, extending the API to support frame base or adding frame base hack might not fly. Perhaps users that want to support VA over V4L2 stateless uAPI (which is more flexible for HW vendor then VA) is to implement a bitstream parser in their VA driver. It's suboptimal, but you can then get all the parameters you need.

pH5 added 4 commits September 5, 2019 15:31

Add dec_ref_pic_marking() syntax element size to VASliceParameterBuff…

6c0b549

…erH264 The Hantro G1 hardware H.264 decoder requires this information. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Add idr_pic_id syntax element to VASliceParameterBufferH264

feed524

The Hantro G1 hardware H.264 decoder requires this information. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Add num_ref_idx_l[01]_default_active_minus1 syntax elements to VAPict…

3856b78

…ureParameterBufferH264 The Hantro G1 hardware H.264 decoder requires this information. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

XinfengZhang reviewed Sep 5, 2019

View reviewed changes

pH5 mentioned this pull request Sep 19, 2019

WIP: Hantro G1 H.264 support bootlin/libva-v4l2-request#26

Open

build: meson: bump api micro version

0b8a288

Bump the API version to 1.6.1 due to the fields added to VAPictureParameterBufferH264 and VASliceParameterBufferH264.

fhvwy reviewed Nov 8, 2020

View reviewed changes

Extend parameter buffers to support Hantro G1 H.264 #332

Are you sure you want to change the base?

Extend parameter buffers to support Hantro G1 H.264 #332

Uh oh!

Conversation

pH5 commented Sep 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CameronNemo commented Oct 5, 2020

Uh oh!

ndufresne commented Oct 13, 2020

Uh oh!

CameronNemo commented Oct 13, 2020

Uh oh!

okias commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ndufresne commented Oct 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

okias commented Oct 13, 2020 •

edited

Loading