GNOME Bugzilla – Bug 757941
vaapiencode: Allow to set profiles via capsfilters
Last modified: 2017-07-04 11:21:10 UTC
Hello, I am doing some work with gstreamer-vaapi on an Intel i3-4370 (Haswell) CPU. I am running libva 1.6.1, and libva Intel driver 1.6.0 on Ubuntu 15.04. The issue is that specifying a profile via a capsfilter, like so: video/x-h264,profile=high does not actually do anything to change the behavior of the encoder. No matter what, the output file is the same size, and if I do something like: avprobe -show_streams $MOVIE It always reports: profile=Constrained Baseline Example pipeline: gst-launch-1.0 filesrc location=/ramdisk/bbb_sunflower_1080p_30fps_normal.mp4 ! qtdemux ! vaapidecode ! vaapiencode_h264 ! video/x-h264,profile=high ! qtmux ! filesink location=/ramdisk/tmp.mov However, if I change the encode element to: vaapiencode_h264 dct8x8=1 or vaapiencode_h264 cabac=1 I do see some changes in the reported profile, as well as size changes in the output file. It seems that the specified profile is acting to restrict what techniques are used, rather than configuring which techniques should be used. My expected behavior is that specifying the profile will cause the encoder to use whatever tools it has at its disposal that are part of that profile. For example, if vainfo tells me that I have: VAProfileH264ConstrainedBaseline: VAEntrypointEncSlice VAProfileH264Main : VAEntrypointEncSlice VAProfileH264High : VAEntrypointEncSlice then, I would expect to simply be able to select one of these profiles from gstreamer-vaapi and get the performance/compression benefits. I am willing to take a shot at fixing this, if someone can point me in the right direction. Thanks, Bill
Further information in this thread of the libva's mailing list http://lists.freedesktop.org/archives/libva/2015-November/003706.html
We will fix this after the 0.7 release. Postponing it for 0.8.
Created attachment 330220 [details] [review] encoder: h264: Use high profile by default A simple solution here is to just change the defaults to enable these features and then just allow the existing caps negotiation disable them if necessary. This is more like how x264enc behaves and also probably the right default settings in most cases anyway.
Comment on attachment 330220 [details] [review] encoder: h264: Use high profile by default Attachment 330220 [details] pushed as 4aec5bd - encoder: h264: Use high profile by default
Few things: -- the patch making the default "tuning" to high compression as a side effect. I mean, for a default pipeline, now there is no difference even if user set tune=high unless there is a manual capsfilter to change the profile as Main :) ---cabac is computational heavy operation which can affect ~30% of performance ,but will give better compression too. So enabling this property to TRUE by default may impact the customers who use the older configurations(who are not manually setting the property). But Scott's patch could be the easiest solution instead of more code re-write in both plugins and libgstvaapi except the "tuning" option behavior i already mentioned.
Enabling B-frames (which is default after applying patch) giving a driver warning "WARNING: Input ref list is Wrong !" . Could be a driver regression. Would be good if someone can investigate? Shouldn't be that difficult I guess.
Regression: https://bugzilla.gnome.org/show_bug.cgi?id=768647#c1
(In reply to sreerenj from comment #6) > Enabling B-frames (which is default after applying patch) giving a driver > warning "WARNING: Input ref list is Wrong !" . Could be a driver regression. > Would be good if someone can investigate? Shouldn't be that difficult I > guess. Sree, I checked into that warning and posted a patch in this bug for it: https://bugzilla.gnome.org/show_bug.cgi?id=768827 I'm not quite following what the right thing to do with the tune property is. Is just changing the default ok? Or is something else required?
Other vaapi encoding controls such as setting MVC encoding modes, could also be set using the upstream caps instead of having to also provide properties to encoding elements (ML ref: https://lists.freedesktop.org/archives/gstreamer-devel/2017-January/062494.html).
Right now the encoders (h264 mainly) chooses its profile given the parameters, and ignores what it is specified in the src caps filter. I guess the logic should be * if no profile is specified, choose the profile as usual * Fix: reflect that profile in the negotiated caps * if the profile is specified in the src caps * Try to set the parameters according to the profile * if it is not possible to figure out which parameter update bail out with an error * process the sink caps to find in the media has multiple views to configure the property that enables MVC.
Created attachment 353051 [details] [review] libs: encoder: h264: changes raw number of profile to macro name of its Changes raw number of profile to macro name of its to improve readability.
Created attachment 353052 [details] [review] encode: h264: set profile to src caps if nothing If nothing for profile is specified in src caps, encoder usually chooses proper profile by itself with parameters such as dct8x8 or cabac. In this case, we need to reflect this profile to src caps.
Created attachment 353053 [details] [review] libs: encoder: h264: set profile via capsfilter Until now, we just ignores profile in src caps and chooses it with given parameters. But it's necessary to honor the profile specifed in src caps to make it easy to configure for users. So this patch does consider high profile as default and start from it to choose right profile with given profile. If given parameters are not compatible with given profile, it will bail out with an error. But for cabac, num of bframes, and dct8x8, it will update them to proper value according to given profile since they are set for high profile as default. Otherwise users should always give specific parameters if they want to use lower profile than high.
(In reply to Víctor Manuel Jáquez Leal from comment #10) > * process the sink caps to find in the media has multiple views to configure > the property that enables MVC. We can continue consdering this and providing preset, which has been discussed with Victor.
Comment on attachment 353051 [details] [review] libs: encoder: h264: changes raw number of profile to macro name of its Attachment 353051 [details] pushed as 3dce2502 - libs: encoder: h264: changes raw number of profile to macro name of its
Review of attachment 353052 [details] [review]: ::: gst/vaapi/gstvaapiencode_h264.c @@ +327,3 @@ + /* Check if there's already profile specified */ + if (!is_profile && gst_structure_has_field_typed (structure, "profile", + G_TYPE_STRING)) The problem here is that the profile can be a GST_TYPE_LIST. What I'm thinking to do here (and in all the encoders) is to fixate the caps
Created attachment 354219 [details] [review] vaapiencode: h264: check for avc in set_config() The check for avc stream format was done in the vaapi encoder's vmethod get_caps(), but that is wrong since it has to be check when encoder set_format().
Created attachment 354220 [details] [review] vaapiencode: h264: set profile in src caps if not specified If no profile is specified in src caps, encoder usually chooses a proper one by itself through the parameters such as dct8x8 or cabac. In this case, we need to set the chosen profile in src caps.
Created attachment 354221 [details] [review] vaapiencode: h264: improve set_config() vmethod First check if downstream requests ANY caps. If so, byte-stream is used and the profile will be choose by the encoder. If dowstream requests EMPTY caps, the negotiation will fail. Lately, byte-stream and profile are looked in the allowed caps.
Created attachment 354222 [details] [review] vaapiencode: h264: verify if requested profile is supported Check if the requested profile in source caps, is supported by the VA driver. If it is not, an info log message is send saying that another (compatible?) profile will be used.
Comment on attachment 353053 [details] [review] libs: encoder: h264: set profile via capsfilter Hyunjun, Please rebase this patch with the patches I posted. Keep in mind that if no max_profile_idc is set, then the encoder will have to choose one accordingly with the properties (as is right now) if max_profile_idc is set and it is the same as the profile_idc, do nothing if max_profile_idc is "bigger" than the profile_idc, modify the profile_idc and adjust the properties. if max_profile_idc is "lower" that the profile_idc... perhaps we should use the profile_idc
(In reply to Víctor Manuel Jáquez Leal from comment #21) > Comment on attachment 353053 [details] [review] [review] > libs: encoder: h264: set profile via capsfilter > > Hyunjun, > > Please rebase this patch with the patches I posted. > > Keep in mind that > > if no max_profile_idc is set, then the encoder will have to choose one > accordingly with the properties (as is right now) > > if max_profile_idc is set and it is the same as the profile_idc, do nothing > > if max_profile_idc is "bigger" than the profile_idc, modify the profile_idc > and adjust the properties. > Okay by here. > if max_profile_idc is "lower" that the profile_idc... perhaps we should use > the profile_idc There are some confusing cases here. 1. vaapih264enc dct8x8=true ! video/x-h264,profile=Main 2. vaapih264enc ! video/x-h264,profile=Main These cases are same internally since dct8x8 is TRUE by default. I thought in the case 1 we should error out. And for the case 2, we should use Main profile and get it working. If you agree with my thought, I think we'd better return default of dct8x8,num_b_frame,use_cabac to the previous, so that we could handle cases easily. What do you think?
I'm testing current 4 patches. The second patch implemented by myself causes a problem when profile is changed. Test pipeline: gst-launch-1.0 videotestsrc num-buffers=100 ! video/x-raw,width=1920,height=1080 ! vaapih264enc ! video/x-h264,profile=baseline ! mp4mux ! fakesink In this pipeline, it changes profile from baseline to constrained-baseline, which is correct. But it fails to push the first frame due to "not-negotiated" probably because it's already negotiated with profile=baseline. Previously this was working with using constrained-baseline profile without changing profile in src caps . What I'm thinking is - Is this right that encoder is working without changing profile of caps? -> I don't think so, that's why we're trying to fix this issue! - Then how can we fix this case? -> we should try to negotiate with updated profile at set_format. What do you think?...again :)
(In reply to Hyunjun Ko from comment #22) > (In reply to Víctor Manuel Jáquez Leal from comment #21) > > Comment on attachment 353053 [details] [review] [review] [review] > > libs: encoder: h264: set profile via capsfilter > > > > Hyunjun, > > > > Please rebase this patch with the patches I posted. > > > > Keep in mind that > > > > if no max_profile_idc is set, then the encoder will have to choose one > > accordingly with the properties (as is right now) > > > > if max_profile_idc is set and it is the same as the profile_idc, do nothing > > > > if max_profile_idc is "bigger" than the profile_idc, modify the profile_idc > > and adjust the properties. > > > Okay by here. > > > if max_profile_idc is "lower" that the profile_idc... perhaps we should use > > the profile_idc > > There are some confusing cases here. > > 1. vaapih264enc dct8x8=true ! video/x-h264,profile=Main > 2. vaapih264enc ! video/x-h264,profile=Main > > These cases are same internally since dct8x8 is TRUE by default. > I thought in the case 1 we should error out. > And for the case 2, we should use Main profile and get it working. I agree. > > If you agree with my thought, I think we'd better return default of > dct8x8,num_b_frame,use_cabac to the previous, so that we could handle cases > easily. > > What do you think? If I understand correctly, what are you saying is to revert 4aec5bdd and then handle these cases??? One more thing: when need to test the stereo profile, in order to see if we don't have a regression there. I haven't tested it.
(In reply to Víctor Manuel Jáquez Leal from comment #24) > > If I understand correctly, what are you saying is to revert 4aec5bdd and > then handle these cases??? Yes. That's what I'm saying. > > One more thing: when need to test the stereo profile, in order to see if we > don't have a regression there. I haven't tested it. Sure.
(In reply to Hyunjun Ko from comment #23) > I'm testing current 4 patches. > The second patch implemented by myself causes a problem when profile is > changed. > > Test pipeline: > gst-launch-1.0 videotestsrc num-buffers=100 ! > video/x-raw,width=1920,height=1080 ! vaapih264enc ! > video/x-h264,profile=baseline ! mp4mux ! fakesink > > In this pipeline, it changes profile from baseline to constrained-baseline, > which is correct. But it fails to push the first frame due to > "not-negotiated" Does the pipeline return a "not-negotiated" error? That might be a correct behavior... isn't it? > probably because it's already negotiated with profile=baseline. > Previously this was working with using constrained-baseline profile without > changing profile in src caps . > > What I'm thinking is > - Is this right that encoder is working without changing profile of caps? > -> I don't think so, that's why we're trying to fix this issue! > - Then how can we fix this case? > -> we should try to negotiate with updated profile at set_format. mmmh... it makes sense. > > What do you think?...again :)
(In reply to Hyunjun Ko from comment #25) > (In reply to Víctor Manuel Jáquez Leal from comment #24) > > > > If I understand correctly, what are you saying is to revert 4aec5bdd and > > then handle these cases??? > Yes. That's what I'm saying. Agreed.
(In reply to Víctor Manuel Jáquez Leal from comment #26) > > In this pipeline, it changes profile from baseline to constrained-baseline, > > which is correct. But it fails to push the first frame due to > > "not-negotiated" > > Does the pipeline return a "not-negotiated" error? > > That might be a correct behavior... isn't it? > I thought that it's correct but according to bug #782949, mp4mux/matroskamux shouldn't be allowed to change them on the fly.
(In reply to Hyunjun Ko from comment #28) > (In reply to Víctor Manuel Jáquez Leal from comment #26) > > > In this pipeline, it changes profile from baseline to constrained-baseline, > > > which is correct. But it fails to push the first frame due to > > > "not-negotiated" > > > > Does the pipeline return a "not-negotiated" error? > > > > That might be a correct behavior... isn't it? > > > > I thought that it's correct but according to bug #782949, mp4mux/matroskamux > shouldn't be allowed to change them on the fly. If I understand it correctly, this is related with the muxers, not the encoder. Other elements might accept it.
(In reply to Víctor Manuel Jáquez Leal from comment #29) > (In reply to Hyunjun Ko from comment #28) > > (In reply to Víctor Manuel Jáquez Leal from comment #26) > > > > In this pipeline, it changes profile from baseline to constrained-baseline, > > > > which is correct. But it fails to push the first frame due to > > > > "not-negotiated" > > > > > > Does the pipeline return a "not-negotiated" error? > > > > > > That might be a correct behavior... isn't it? > > > > > > > I thought that it's correct but according to bug #782949, mp4mux/matroskamux > > shouldn't be allowed to change them on the fly. > > If I understand it correctly, this is related with the muxers, not the > encoder. Other elements might accept it. I investigated into this issue, and this is fault on gst-vaapi. For this case, gst encoder tries to change from baseline to constrained-profile, which was not on the caps when linked to downstream. IIUC, we can change profile only on the allowed caps when trying to re-nego.
Created attachment 354541 [details] [review] vaapiencode: h264: set profile to src caps So far vaapi encoder does not set profile to src caps. This patch makes it setting profile to src caps, which is determined by itself. In addition, if encoder chose different profile, which is not negotiated with downstream, we should set compatible profile to make negotiation working. ---------------------------- I would like to propose new patch for the second patch on the list.
Created attachment 354544 [details] [review] libs: encoder: h264: set profile via capsfilter Until now, we just ignores profile in src caps and chooses it with given parameters. But it's necessary to honor the profile specifed in src caps to make it easy to configure for users. So this patch does the following to choose right profile with given profile. 1\ If given parameters are not compatible with given profile, it will bail out with an error. 2\ If it can encode with higher profile based on src caps, get it higher.
Created attachment 354545 [details] [review] Revert "encoder: h264: Use high profile by default" This reverts commit 4aec5bdd7207fc0e45813ef14c9c0ad5174a8f75.
Please make it obsolete: https://bugzilla.gnome.org/attachment.cgi?id=354220 And review this instead, which fixes "not-negotiated" problem. https://bugzilla.gnome.org/attachment.cgi?id=354541
Attachment 354219 [details] pushed as b713af4 - vaapiencode: h264: check for avc in set_config() Attachment 354221 [details] pushed as 322fe98 - vaapiencode: h264: improve set_config() vmethod Attachment 354222 [details] pushed as 39b36f7 - vaapiencode: h264: verify if requested profile is supported Attachment 354541 [details] pushed as d018f64 - vaapiencode: h264: set profile to src caps Attachment 354544 [details] pushed as ca84fd2 - libs: encoder: h264: set profile via capsfilter Attachment 354545 [details] pushed as 5b38e7f - Revert "encoder: h264: Use high profile by default"
*** Bug 780509 has been marked as a duplicate of this bug. ***