[FFmpeg-devel,2/4] lavc/qsvenc: fix hevc vps extradata issues

Submitted by Zhong Li on March 26, 2019, 7:46 p.m.

Details

Message ID 20190326194651.7121-2-zhong.li@intel.com
State New
Headers show

Commit Message

Zhong Li March 26, 2019, 7:46 p.m.
cbs trace qsv vps header failed due to some reasons:
1. vps_temporal_id_nesting_flag is not set but spec required it must to
   be 1 when vps_max_sub_layers_minus1 is equal to 0.
2. vps_num_hrd_parameters is not set and written.
3. other issues in ff_hevc_encode_nal_vps() (have fixed in pervious commit).

Reproduce: ffmpeg -hwaccel qsv -v verbose -c:v h264_qsv -i bbb_sunflower_1080p_30fps_normal.mp4 -vframes 1
-c:v hevc_qsv  -bsf:v trace_headers -f null -

Signed-off-by: Zhong Li <zhong.li@intel.com>
---
 libavcodec/qsvenc_hevc.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Comments

Mark Thompson March 27, 2019, 10:06 p.m.
On 26/03/2019 19:46, Zhong Li wrote:
> cbs trace qsv vps header failed due to some reasons:
> 1. vps_temporal_id_nesting_flag is not set but spec required it must to
>    be 1 when vps_max_sub_layers_minus1 is equal to 0.
> 2. vps_num_hrd_parameters is not set and written.
> 3. other issues in ff_hevc_encode_nal_vps() (have fixed in pervious commit).
> 
> Reproduce: ffmpeg -hwaccel qsv -v verbose -c:v h264_qsv -i bbb_sunflower_1080p_30fps_normal.mp4 -vframes 1
> -c:v hevc_qsv  -bsf:v trace_headers -f null -
> 
> Signed-off-by: Zhong Li <zhong.li@intel.com>
> ---
>  libavcodec/qsvenc_hevc.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)

I had a patch sitting around for a long time for this - see <https://lists.libav.org/pipermail/libav-devel/2017-December/085498.html>.  It just deletes all of the ad-hoc writing code and uses the tested CBS paths instead, which have the additional advantage of correctly preserving various things which the existing code doesn't cover at all (e.g. newer PTL flags which didn't exist when this was written).

The main problem with it was that I didn't have enough libmfx platforms at the time to be confident in testing of it, so it got stalled.

Would it be useful to resurrect that?

- Mark
Zhong Li March 28, 2019, 11:36 a.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Mark Thompson

> Sent: Thursday, March 28, 2019 6:06 AM

> To: ffmpeg-devel@ffmpeg.org

> Subject: Re: [FFmpeg-devel] [PATCH 2/4] lavc/qsvenc: fix hevc vps extradata

> issues

> 

> On 26/03/2019 19:46, Zhong Li wrote:

> > cbs trace qsv vps header failed due to some reasons:

> > 1. vps_temporal_id_nesting_flag is not set but spec required it must to

> >    be 1 when vps_max_sub_layers_minus1 is equal to 0.

> > 2. vps_num_hrd_parameters is not set and written.

> > 3. other issues in ff_hevc_encode_nal_vps() (have fixed in pervious

> commit).

> >

> > Reproduce: ffmpeg -hwaccel qsv -v verbose -c:v h264_qsv -i

> > bbb_sunflower_1080p_30fps_normal.mp4 -vframes 1 -c:v hevc_qsv

> -bsf:v

> > trace_headers -f null -

> >

> > Signed-off-by: Zhong Li <zhong.li@intel.com>

> > ---

> >  libavcodec/qsvenc_hevc.c | 16 +++++++++-------

> >  1 file changed, 9 insertions(+), 7 deletions(-)

> 

> I had a patch sitting around for a long time for this - see

> <https://lists.libav.org/pipermail/libav-devel/2017-December/085498.html>.

> It just deletes all of the ad-hoc writing code and uses the tested CBS paths

> instead, which have the additional advantage of correctly preserving various

> things which the existing code doesn't cover at all (e.g. newer PTL flags

> which didn't exist when this was written).

> 

> The main problem with it was that I didn't have enough libmfx platforms at

> the time to be confident in testing of it, so it got stalled.

> 

> Would it be useful to resurrect that?

> 

> - Mark


MSDK can support vps extra_data since V1.17. Actually now it have been moved to V1.29.
So maybe we can consider the lightweight way to fix the fake vps error issue, instead of introduce cbs.

Using cbs can be a good idea if we decided to delete ff_hevc_encode_nal_vps() totally (IHMO, qsv hevc encoder is the only user of ff_hevc_encode_nal_vps()).

Patch hide | download patch | download mbox

diff --git a/libavcodec/qsvenc_hevc.c b/libavcodec/qsvenc_hevc.c
index 3d7e9cfba4..edc9a8ab54 100644
--- a/libavcodec/qsvenc_hevc.c
+++ b/libavcodec/qsvenc_hevc.c
@@ -107,6 +107,7 @@  static int generate_fake_vps(QSVEncContext *q, AVCodecContext *avctx)
     /* generate the VPS */
     vps.vps_max_layers     = 1;
     vps.vps_max_sub_layers = sps.max_sub_layers;
+    vps.vps_temporal_id_nesting_flag = sps.temporal_id_nesting_flag;
     memcpy(&vps.ptl, &sps.ptl, sizeof(vps.ptl));
     vps.vps_sub_layer_ordering_info_present_flag = 1;
     for (i = 0; i < HEVC_MAX_SUB_LAYERS; i++) {
@@ -120,7 +121,8 @@  static int generate_fake_vps(QSVEncContext *q, AVCodecContext *avctx)
     vps.vps_num_units_in_tick               = sps.vui.vui_num_units_in_tick;
     vps.vps_time_scale                      = sps.vui.vui_time_scale;
     vps.vps_poc_proportional_to_timing_flag = sps.vui.vui_poc_proportional_to_timing_flag;
-    vps.vps_num_ticks_poc_diff_one          = sps.vui.vui_num_ticks_poc_diff_one_minus1 + 1;
+    vps.vps_num_ticks_poc_diff_one      = sps.vui.vui_num_ticks_poc_diff_one_minus1 + 1;
+    vps.vps_num_hrd_parameters = 0;
 
     /* generate the encoded RBSP form of the VPS */
     ret = ff_hevc_encode_nal_vps(&vps, sps.vps_id, vps_rbsp_buf, sizeof(vps_rbsp_buf));
@@ -138,12 +140,12 @@  static int generate_fake_vps(QSVEncContext *q, AVCodecContext *avctx)
     bytestream2_put_byte(&pbc, 1);                 // header
 
     while (bytestream2_get_bytes_left(&gbc)) {
-        uint32_t b = bytestream2_peek_be24(&gbc);
-        if (b <= 3) {
-            bytestream2_put_be24(&pbc, 3);
-            bytestream2_skip(&gbc, 2);
-        } else
-            bytestream2_put_byte(&pbc, bytestream2_get_byte(&gbc));
+	if (bytestream2_get_bytes_left(&gbc) >= 3 &&
+		bytestream2_peek_be24(&gbc) <= 3) {
+	    bytestream2_put_be24(&pbc, 3);
+	    bytestream2_skip(&gbc, 2);
+	} else
+	    bytestream2_put_byte(&pbc, bytestream2_get_byte(&gbc));
     }
 
     vps_size = bytestream2_tell_p(&pbc);