GNOME Bugzilla – Bug 796984
vaapih265enc: Support hevc encode low latency B frame.
Last modified: 2018-08-29 13:02:40 UTC
Some Intel hardware doesn't hevc P frame encode, so need low latency B feature to transform P frame into predictive B frame.
Created attachment 373367 [details] [review] enable hevc low latency B.
Review of attachment 373367 [details] [review]: Hi Fei, Perhaps we need to complete the commit log information: What Intel hardware? using which vaapi backend? ::: gst-libs/gst/vaapi/gstvaapiencoder_h265.c @@ +824,3 @@ WRITE_UINT32 (bs, + slice_param->slice_fields. + bits.slice_loop_filter_across_slices_enabled_flag, 1); this change is related with code style, not with the purpose of this patch. I would remove it. @@ +1748,3 @@ + slice_param->slice_fields. + bits.slice_loop_filter_across_slices_enabled_flag = TRUE; ditto. @@ +2769,3 @@ + "Low latency B will change P frame into predictive B frame", + FALSE, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); + I'm not very fond to add properties for every hardware specific requirement, because properties are like APIs, we have to keep them, and maintain them, as soon as they are public almost forever. Are you sure there isn't a way to query vaapi backend or an API to query the chipset and set all this hacks???
@Victor, update my commit log. Currently all our existed platforms support P and low latency B frame. And I cann't provide correct information if the next platform will also support P frame. So I will not mention the hardware info in commit log. And both vaapi-driver and media-driver support this. There is a new interface in libva to provide query: https://github.com/intel/libva/pull/220, if the patch merged(need driver to support it), we can write another patch to query if the hardware support it before do the low latency B. But I think the property is needed, now all our existed platforms support both P and low latency B, there should be a property to let user choose the right path.
Created attachment 373411 [details] [review] enable hevc low latency B.
Hi Fei, (In reply to Fei from comment #3) > @Victor, update my commit log. Currently all our existed platforms support P > and low latency B frame. And I cann't provide correct information if the > next platform will also support P frame. So I will not mention the hardware > info in commit log. And both vaapi-driver and media-driver support this. OK. What about the unrelated code-style changes? > There is a new interface in libva to provide query: > https://github.com/intel/libva/pull/220, if the patch merged(need driver to > support it), we can write another patch to query if the hardware support it > before do the low latency B. But I think the property is needed, now all our > existed platforms support both P and low latency B, there should be a > property to let user choose the right path. I would wait for the patch to get merged. Otherwise I don't want to follow that path that would overpopulate the properties for particular architectures. But if we do, I would prefix them, as intel-*, in this case intel-low-latency-b In my opinion, we need a third opinion on this case.
Created attachment 373414 [details] [review] enable hevc low latency B.
Hi Victor, I have remove unrelated code change. And change the option into intel-low-latency-b. @Sree, could you give your opinion on this? Thanks.
No, I don't think it is good idea to add a prefix of intel-. low-delay b frame is not intel specific.
Review of attachment 373414 [details] [review]: lgtm though we would need to reach a consensus on the property prefix and if the property is required at all
(In reply to Víctor Manuel Jáquez Leal from comment #9) > Review of attachment 373414 [details] [review] [review]: > > lgtm > > though we would need to reach a consensus on the property prefix and if the > property is required at all Hi Victor, Just as Haihao's comment, low delay B frame is not intel specific, so I think it's better to remove prefix Intel. Otherwise, it will confused. How do you think of this ?
(In reply to Fei from comment #10) > (In reply to Víctor Manuel Jáquez Leal from comment #9) > > Review of attachment 373414 [details] [review] [review] [review]: > > > > lgtm > > > > though we would need to reach a consensus on the property prefix and if the > > property is required at all > > Hi Victor, > Just as Haihao's comment, low delay B frame is not intel specific, so I > think it's better to remove prefix Intel. Otherwise, it will confused. How > do you think of this ? I agree it is not Intel exclusive, but still it is very driver backend specific. What would happen if users of other backends enables it breaking their uses cases? We would document very carefully and exactly what does this property means and why the use should enable it or not.
Another question is, if https://github.com/intel/libva/pull/220 is merged, this property should be moved to the base encoder class, since it will apply to all the codecs? Is there any use case where a user would want to enable this option even it their backend supports P frames?
Created attachment 373450 [details] [review] enable hevc low latency B.
(In reply to Víctor Manuel Jáquez Leal from comment #12) > Another question is, if https://github.com/intel/libva/pull/220 is merged, > this property should be moved to the base encoder class, since it will apply > to all the codecs? > > Is there any use case where a user would want to enable this option even it > their backend supports P frames? It won't break other backend after the libva patch merged. And we need to query if hardware support low latency B in gst-vaapi, if not, then break out and print to user the reason. We can consider to move this property to base class in next step. AFAIK, this feature only benefit on none-p frame supported platforms.
@Victor, If no other comment, could you help to review and merge this patch? Thanks.
@Fei: Could you please explain a bit about the logic behind the low-delay-b implementation? What I can see is that the patch just changes the slice type from P to B if low-delay-b enabled. Is it supposed to maintain 2 ref pic lists (list_0 and list_1) in this case?
(In reply to sreerenj from comment #16) > @Fei: Could you please explain a bit about the logic behind the low-delay-b > implementation? > > What I can see is that the patch just changes the slice type from P to B if > low-delay-b enabled. Is it supposed to maintain 2 ref pic lists (list_0 and > list_1) in this case? Yes, In this case the 2 ref pic lists is needed. I guess driver will do low delay b when it detect frame type is B and there are 2 ref pic lists. And I encoded a yuv into h265 stream with low-latency-b enabled, and checked the stream with intel-video-analyzer, all P frame changed into predictive B which is expected. Here is reference code(option p2b) from libva-utils: https://github.com/intel/libva-utils/pull/116/
Review of attachment 373450 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiencoder_h265.c @@ +1721,1 @@ for (; i_ref < reflist_1_count; ++i_ref) { Will reflist_1_count be always zero here for any P picture ? (https://bugzilla.gnome.org/show_bug.cgi?id=796984#c17)
(In reply to sreerenj from comment #18) > Review of attachment 373450 [details] [review] [review]: > > ::: gst-libs/gst/vaapi/gstvaapiencoder_h265.c > @@ +1721,1 @@ > for (; i_ref < reflist_1_count; ++i_ref) { > > Will reflist_1_count be always zero here for any P picture ? > (https://bugzilla.gnome.org/show_bug.cgi?id=796984#c17) Sree hit something important, what would be the behavior of this property when the property max-bframes is zero (the default value, by the way)
Review of attachment 373450 [details] [review]: I would recommend you to explain, in the commit log, the purpose of this patch a bit longer. At least answer these questions: Why is this change necessary? How does this commit address the issue? What effects does this change have?
Review of attachment 373450 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiencoder_h265.c @@ +2768,3 @@ + g_param_spec_boolean ("low-latency-b", + "Enable low latency b", + "Low latency B provide function to transform P frame into predictive B frame.This function can be used when platform doen't support P frame.", There are a couple typos. I would rephrase this line as "Transforms P frames into predictive B frames. Enable it when P frames are not supported"
(In reply to Víctor Manuel Jáquez Leal from comment #19) > (In reply to sreerenj from comment #18) > > Review of attachment 373450 [details] [review] [review] [review]: > > > > ::: gst-libs/gst/vaapi/gstvaapiencoder_h265.c > > @@ +1721,1 @@ > > for (; i_ref < reflist_1_count; ++i_ref) { > > > > Will reflist_1_count be always zero here for any P picture ? > > (https://bugzilla.gnome.org/show_bug.cgi?id=796984#c17) > > Sree hit something important, what would be the behavior of this property > when the property max-bframes is zero (the default value, by the way) Yes, low latency B's ref_pic_list1 should keep same with ref_pic_list0 of P frame but not B frame. I will fix this in my next patch.
Created attachment 373490 [details] [review] enable hevc low latency B. @Victor, fixed your two comments, please help review this again.
Could you use 'low-delay b frame' instead of 'low latency b frame' in the patch? I can find something about 'low-delay b frame' by Google, but none for 'low latency b frame'.
Created attachment 373491 [details] [review] enable hevc low delay B. use low delay B instead of low latency B.
(In reply to haihao.xiang from comment #24) > Could you use 'low-delay b frame' instead of 'low latency b frame' in the > patch? I can find something about 'low-delay b frame' by Google, but none > for 'low latency b frame'. Fixed.
Review of attachment 373491 [details] [review]: lgtm
attachment 373491 [details] [review] was pushed as commit 2fce126c - libs: encoder: h265: add low delay B frame support.