GNOME Bugzilla – Bug 783803
encoder: h264(AVC): Add multi reference support
Last modified: 2017-08-02 09:32:53 UTC
Currently we only support 1 ref frame in P frame encode and 2 ref frames in B frame encode. With the recent changes in Intel-vaapi-driver(SKL, BXT and KBL platforms), we should be able to use multi-referencing. -- Query the driver with VAConfigAttribEncMaxRefFrames and find the number of reference frames supported. Need a fix in driver:Currently the value is hard-coded in driver query implementation. -- Implement a basic algorithm in vaapih264encoder to utilize the multi referencing. For eg: use the previous frames(n-1, n-2...n-x, where n=current frame, x=user-input-number-of-ref-frames-to-be-used) for referencing.
@Sree, I have one question. If one property is provided for number of reference frames to be set, How do I decide the number of reference frames for picture list 0 and list 1? Currently query results in 8 for list0 and 2 for list1, which means that if b-frame is encoded, we can add one or two reference frames to the pic list1. Possible options I think so far are 1\ the value of the propery is considered as the number of picture list0. and add 1 reference frame more if b-frame encoding. 2\ Bit operation can be used just like the value of the query VAConfigAttribEncMaxRefFrames the driver. 3\ Follow the libva-utils example. But I doubt if it's correct. If there's something I misunderstand, please let me know.
(In reply to Hyunjun Ko from comment #1) The question here is: How we interpret the number of future reference frames if there are B frames encoded. right? A: The simple scenario is what you mentioned in \1. Option \2 will create confusion, So I won't prefer even though that is what intel-vaapi-driver is doing. B: Look what x264enc is doing if we set "ref=n" and do the same with hw encoder. c: Third option: You can add different algorithms and enable them based on a property. For eg: See how vaapivp9enc implemented the "ref-pic-mode". Here we can enable "multi-ref-b" as one of the options. You can take few patches from here I believe:https://cgit.freedesktop.org/~sree/gstreamer-vaapi/log/?h=advanced_enc Specifically this one: https://cgit.freedesktop.org/~sree/gstreamer-vaapi/commit/?h=advanced_enc&id=338e5d3a54f660acd628e4eeff336312c93bf0d8
Created attachment 356499 [details] [review] libs: encoder: implements gst_vaapi_encoder_ensure_max_num_ref_frames This function will query VAConfigAttribEncMaxRefFrames to get the maximum number of reference frames supported in the driver. This will be used for h264/h265 encoding.
Created attachment 356500 [details] [review] libs: encoder: h264: add num-ref-frames property Users can provide the number of reference frame by this property. The value of the property will be considered as the number of reference picture list0 and will add 1 reference frame more to the reference picture list1 internally if b-frame encoding. If the value provided is bigger than the number of refrence frames supported in the driver, it will be lowered.
Created attachment 356501 [details] [review] libs: encoder: h264: add multi reference support Using num_ref_frames provided and the result of the Query VAConfigAttribEncMaxRefFrames, it determines the size of reference list and perform encoding with multi reference frames as the following: 1\ The num_ref_frames is being considered as the number of reference picture list0 2\ Encoder adds 1 reference frame more to the reference picture list1 internally if b-frame encoding. 3\ If num_ref_frames is bigger than the number of refrence frames supported in the driver, it will be lowered.
Thanks for comment! I choosed the A option since it's the simplest way. I beleive that we can add something you mentioned about C option based on the proposed patches. These patches are a sort of RFC. So I need comments and review. @Sree, IIUC, the commit below is worth to be merged. What do you think? encoder: h264: Fix frame_num generation (https://cgit.freedesktop.org/~sree/gstreamer-vaapi/commit/?h=advanced_enc&id=b3f9f31a4dc356e8ad6537477c5fc8b17a20d306) (In reply to sreerenj from comment #2) > (In reply to Hyunjun Ko from comment #1) > > The question here is: How we interpret the number of future reference frames > if there are B frames encoded. right? > > A: > The simple scenario is what you mentioned in \1. > Option \2 will create confusion, So I won't prefer even though that is what > intel-vaapi-driver is doing. > > B: > Look what x264enc is doing if we set "ref=n" and do the same with hw encoder. > > c: > Third option: You can add different algorithms and enable them based on a > property. For eg: See how vaapivp9enc implemented the "ref-pic-mode". Here > we can enable "multi-ref-b" as one of the options. > You can take few patches from here I > believe:https://cgit.freedesktop.org/~sree/gstreamer-vaapi/log/ > ?h=advanced_enc > Specifically this one: > https://cgit.freedesktop.org/~sree/gstreamer-vaapi/commit/ > ?h=advanced_enc&id=338e5d3a54f660acd628e4eeff336312c93bf0d8
Review of attachment 356499 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiencoder.c @@ +1532,3 @@ +gboolean +gst_vaapi_encoder_ensure_max_num_ref_frames (GstVaapiEncoder * encoder, + GstVaapiProfile profile, GstVaapiEntrypoint entrypoint) Perhaps I'm missing something, but encoder has GstVaapiContext and it keeps the profile and entrypoint, so there is no need to pass them. ::: gst-libs/gst/vaapi/gstvaapiencoder_priv.h @@ +417,3 @@ guint media_max_slices, guint * num_slices); +G_GNUC_INTERNAL I did the same mistake in gst_vaapi_encoder_ensure_num_slices() I shall fix it.
Review of attachment 356500 [details] [review]: lgtm
Review of attachment 356501 [details] [review]: lgtm
Review of attachment 356501 [details] [review]: lgtm... but I would like to test it before merge it ;)
(In reply to Víctor Manuel Jáquez Leal from comment #7) > Review of attachment 356499 [details] [review] [review]: > > ::: gst-libs/gst/vaapi/gstvaapiencoder.c > @@ +1532,3 @@ > +gboolean > +gst_vaapi_encoder_ensure_max_num_ref_frames (GstVaapiEncoder * encoder, > + GstVaapiProfile profile, GstVaapiEntrypoint entrypoint) > > Perhaps I'm missing something, but encoder has GstVaapiContext and it keeps > the profile and entrypoint, so there is no need to pass them. > > ::: gst-libs/gst/vaapi/gstvaapiencoder_priv.h > @@ +417,3 @@ > guint media_max_slices, guint * num_slices); > > +G_GNUC_INTERNAL > > I did the same mistake in gst_vaapi_encoder_ensure_num_slices() I shall fix > it. This was the first problem since I started working on this issue. I thought that gst_vaapi_encoder_ensure_num_slices had same problem. The function reset_properties is executed before set_context_info, which means GstVaapiContext is not completed at this moment. But obviously encoder needs the information of the query result at this moment.
(In reply to Víctor Manuel Jáquez Leal from comment #10) > Review of attachment 356501 [details] [review] [review]: > > lgtm... but I would like to test it before merge it ;) I agree. I just tested by mediainfo tool and confirmed the description of the produced h264 raw data. Also watched whether it plays fine by ffmpeg, cvlc, gst-play. Is there any other something good for test?
(In reply to Hyunjun Ko from comment #11) > (In reply to Víctor Manuel Jáquez Leal from comment #7) > > Review of attachment 356499 [details] [review] [review] [review]: > > > > ::: gst-libs/gst/vaapi/gstvaapiencoder.c > > @@ +1532,3 @@ > > +gboolean > > +gst_vaapi_encoder_ensure_max_num_ref_frames (GstVaapiEncoder * encoder, > > + GstVaapiProfile profile, GstVaapiEntrypoint entrypoint) > > > > Perhaps I'm missing something, but encoder has GstVaapiContext and it keeps > > the profile and entrypoint, so there is no need to pass them. > > > > ::: gst-libs/gst/vaapi/gstvaapiencoder_priv.h > > @@ +417,3 @@ > > guint media_max_slices, guint * num_slices); > > > > +G_GNUC_INTERNAL > > > > I did the same mistake in gst_vaapi_encoder_ensure_num_slices() I shall fix > > it. > > This was the first problem since I started working on this issue. > I thought that gst_vaapi_encoder_ensure_num_slices had same problem. > The function reset_properties is executed before set_context_info, which > means GstVaapiContext is not completed at this moment. But obviously encoder > needs the information of the query result at this moment. I'm stupid. That's is described in both functions' documentation.
Review of attachment 356501 [details] [review]: lgtm... but I would like to test it before merge it ;) ::: gst-libs/gst/vaapi/gstvaapiencoder_h264.c @@ +2518,3 @@ + + if (base_encoder->max_num_ref_frames_1 < 1) { + if (encoder->num_bframes > 0) { this can be simplified by using an "&&" operator
Review of attachment 356501 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiencoder_h264.c @@ +2861,3 @@ + base_encoder->num_ref_frames = (encoder->num_ref_frames + + (encoder->num_bframes > 0) + DEFAULT_SURFACES_COUNT) personally I prefer: (encoder->num_bframes > 0 ? 1 : 0) making explicit the use of integers, rather than implicitly cast a boolean value. I think it is more readable.
I would like to merge this ASAP, but I would like to have Sree's OK using this "simple" algorithm for multi reference support. Also, I would like to ask if wouldn't we use the property name "ref" as x264enc?
(In reply to Hyunjun Ko from comment #6) > Thanks for comment! > > I choosed the A option since it's the simplest way. > I beleive that we can add something you mentioned about C option based on > the proposed patches. > These patches are a sort of RFC. So I need comments and review. > > @Sree, IIUC, the commit below is worth to be merged. What do you think? > encoder: h264: Fix frame_num generation > (https://cgit.freedesktop.org/~sree/gstreamer-vaapi/commit/ > ?h=advanced_enc&id=b3f9f31a4dc356e8ad6537477c5fc8b17a20d306) > Yup, But Make sure it is not breaking anything. I haven't tested all use cases, especially MVC. Compare the output with x264enc output, I mean check the generated frame numbers of both.
For this kind of features, It is easy to do the verification If you have some codecanalyzer. But most of them are paid. You may get a free trial version for codecvisa: http://www.codecian.com/
(In reply to Víctor Manuel Jáquez Leal from comment #16) > I would like to merge this ASAP, but I would like to have Sree's OK using > this "simple" algorithm for multi reference support. > > Also, I would like to ask if wouldn't we use the property name "ref" as > x264enc? The name "ref" is better to use to me at least.
(In reply to sreerenj from comment #18) > For this kind of features, It is easy to do the verification If you have > some codecanalyzer. But most of them are paid. > > You may get a free trial version for codecvisa: http://www.codecian.com/ Thanks for introducing this tool. I verified with this tool, seems correct. :)
Created attachment 356763 [details] [review] libs: encoder: h264: add refs property Users can provide the number of reference frame by this property. The value of the property will be considered as the number of reference picture list0 and will add 1 reference frame more to the reference picture list1 internally if b-frame encoding. If the value provided is bigger than the number of refrence frames supported in the driver, it will be lowered.
Created attachment 356764 [details] [review] libs: encoder: h264: add multi reference support Using num_ref_frames provided and the result of the Query VAConfigAttribEncMaxRefFrames, it determines the size of reference list and perform encoding with multi reference frames as the following: 1\ The num_ref_frames is being considered as the number of reference picture list0 2\ Encoder adds 1 reference frame more to the reference picture list1 internally if b-frame encoding. 3\ If num_ref_frames is bigger than the number of refrence frames supported in the driver, it will be lowered.
Attachment 356763 [details] pushed as cd6a973 - libs: encoder: h264: add refs property Attachment 356764 [details] pushed as cdaf15b - libs: encoder: h264: add multi reference support Attachment 356499 [details] pushed as ec76a9a - libs: encoder: implements gst_vaapi_encoder_ensure_max_num_ref_frames