Could be related to bug #719704 too. Adding to list of bugs to fix by 0.5.8 time frame. Thanks.

Created attachment 263634 [details] [review]
set default profile to constrained baseline

GEN driver changed H.264 encoder profile support from baseline to contrained-baseline. the patch add h264_contrained_baseline profile and set H.264 encoder default profile to constrained_baseline. At the same time to keep old driver working, would fallback to baseline if doesn't detect contrained_baseline.

please review

It works in gst10_AVC encoding now.

It still reproduced with the commit:
git://gitorious.org/vaapi/gstreamer-vaapi.git master 61f6cbffc657fbe0287301018e365cd85607565c

Created attachment 264048 [details] [review]
0001-h264-check-constrained-profile-for-encoder

update patch since GST_VAAPI_PROFILE_H264_CONSTRAINED_BASELINE was already added in profile.
please review

No recurrence again, but without applying your patch.
Testing Environment:
Libva:          (master)88ed1ebe960b1c4a7970e12f8df1ed7d7031352a
Libva_intel_driver:             (master)355ce947511b9c249c4e39b006a1aeb7aff5fe17
Gstreamer10:            (1.0)4e880d4d1e151ea64f83c28b5c3e1bbc06c57903
Gst_plugins_base10:             (1.0)2dd3f028c1e6dea799d7496639f53220818b20b1
Gst_plugins_good10:             (1.0)643d425f51f81b56deec16c01162637546708ee5
Gst_plugins_bad10:              (1.0)0587ab41b4f9979e9cfc11011ed5c970569ee3d3
Gst_plugins_ugly10:             (1.0)c7c911b8320576429e4a4234a1e29ec7436e6814
Gst_plugins_vaapi10:            (master)bff53fc5ce36008c209ee93d8d6426d19238a30f

Hi Gwenole,
  How do you think about this patch? since this bug blocked QA's conformance tests, they can't test encoder in master branch now. and this bug has been there for a long time, and other bug fix(patch) also depends on this patch.could u review this patch?

Thanks,
Wind

Review of attachment 264048 [details] [review]:

Try to maintain gst-indent style please. Otherwise, OK. :)

::: gst-libs/gst/vaapi/gstvaapiencoder_h264.c
@@ +189,3 @@
 }
 
+static gboolean _set_profile(GstVaapiEncoderH264 * encoder)

set_profile() please, i.e. no leading '_' chars in identifier names.

@@ +205,3 @@
+  if (!gst_vaapi_display_has_encoder(display,encoder->profile,entrypoint))
+    return FALSE;
+

We probably could do something similar to the decoder part:

GstVaapiProfile profile, profiles[2];
guint n_profiles = 0;

// determine profile based on params
// ...
profiles[n_profiles++] = profile;

switch (profile) {
case constrained-baseline:
  profiles[n_profiles++] = baseline;
  break;
case main:
  profiles[n_profiles++] = high;
  break;
}

// final loop to validate the profile.
and return accordingly.

(In reply to comment #9)
> Review of attachment 264048 [details] [review]:
> 
> Try to maintain gst-indent style please. Otherwise, OK. :)
Different gst-indent generate different files.
Which version are you using? I just take a try(gstreamer/tools/gst-indent, 1.0 branch) but got whole file different.

Could you add a gst-indent in gstreamer-vaapi, then we can make thing easily.

> 
> ::: gst-libs/gst/vaapi/gstvaapiencoder_h264.c
> @@ +189,3 @@
>  }
> 
> +static gboolean _set_profile(GstVaapiEncoderH264 * encoder)
> 
> set_profile() please, i.e. no leading '_' chars in identifier names.
> 
> @@ +205,3 @@
> +  if (!gst_vaapi_display_has_encoder(display,encoder->profile,entrypoint))
> +    return FALSE;
> +
> 
> We probably could do something similar to the decoder part:
> 
> GstVaapiProfile profile, profiles[2];
> guint n_profiles = 0;
> 
> // determine profile based on params
> // ...
> profiles[n_profiles++] = profile;
> 
> switch (profile) {
> case constrained-baseline:
>   profiles[n_profiles++] = baseline;
>   break;
> case main:
>   profiles[n_profiles++] = high;
>   break;
> }
> 
> // final loop to validate the profile.
> and return accordingly.
This can be found in another patch to fix https://bugzilla.gnome.org/show_bug.cgi?id=719694.

(In reply to comment #10)
> (In reply to comment #9)
> > Review of attachment 264048 [details] [review] [details]:
> > 
> > Try to maintain gst-indent style please. Otherwise, OK. :)
> Different gst-indent generate different files.
> Which version are you using? I just take a try(gstreamer/tools/gst-indent, 1.0
> branch) but got whole file different.
> 
> Could you add a gst-indent in gstreamer-vaapi, then we can make thing easily.

I always use the one from the git master branch of the common/ subdirectory. I have noticed that it could only reliably work for .c files. For header files, I usually have to postprocess as GNU indent does not really know about the glib-style GCC attributes. I think we could fix it in some ways.

> > ::: gst-libs/gst/vaapi/gstvaapiencoder_h264.c
> > @@ +189,3 @@
> >  }
> > 
> > +static gboolean _set_profile(GstVaapiEncoderH264 * encoder)
> > 
> > set_profile() please, i.e. no leading '_' chars in identifier names.
> > 
> > @@ +205,3 @@
> > +  if (!gst_vaapi_display_has_encoder(display,encoder->profile,entrypoint))
> > +    return FALSE;
> > +
> > 
> > We probably could do something similar to the decoder part:
> > 
> > GstVaapiProfile profile, profiles[2];
> > guint n_profiles = 0;
> > 
> > // determine profile based on params
> > // ...
> > profiles[n_profiles++] = profile;
> > 
> > switch (profile) {
> > case constrained-baseline:
> >   profiles[n_profiles++] = baseline;
> >   break;
> > case main:
> >   profiles[n_profiles++] = high;
> >   break;
> > }
> > 
> > // final loop to validate the profile.
> > and return accordingly.
> This can be found in another patch to fix
> https://bugzilla.gnome.org/show_bug.cgi?id=719694.

Please provide an appropriate patch with the required changes. Thanks. I could not find that in the other patch.

Created attachment 264830 [details] [review]
0001-encoder-h264-looking-for-correct-profile-to-set

please review. thx

After I applied this patch to gst-vappi, the Y-PSNR value is too low which results in failure in encoding.

Hi Zhenxiang, please try PRC branch git://gitorious.org/gst-plugins-vaapi/gst-plugins-vaapi.git to test the PSNR.
the possibility of your low result caused by bug #719820.
I have tried patch compare with libva avcenc test, Y-PSNR exactly the same. only U,V a little less.

PRC branch do not have the problem, do i need to test it with your patch?
(In reply to comment #14)
> Hi Zhenxiang, please try PRC branch
> git://gitorious.org/gst-plugins-vaapi/gst-plugins-vaapi.git to test the PSNR.
> the possibility of your low result caused by bug #719820.
> I have tried patch compare with libva avcenc test, Y-PSNR exactly the same.
> only U,V a little less.

my patch already in PRC branch.(In reply to comment #15)
> PRC branch do not have the problem, do i need to test it with your patch?
My patch already in PRC branch. you only need to make sure PRC branch doesn't have this issue. the PSNR issue most possibly caused by bug #719820. some patches have dependency with each other, if Gwenole doesn't accept one, normally other patches behind this one would fail too. It's hard for you to always cherry-pick one patch and install into the tree. If you really want to test this tree, sometimes you need to pick up multiple patches before patch merged. this looks very complicated. So I suggest you only need to test PRC branch.

> (In reply to comment #14)
> > Hi Zhenxiang, please try PRC branch
> > git://gitorious.org/gst-plugins-vaapi/gst-plugins-vaapi.git to test the PSNR.
> > the possibility of your low result caused by bug #719820.
> > I have tried patch compare with libva avcenc test, Y-PSNR exactly the same.
> > only U,V a little less.

No such problem in PRC branch, testing with the latest gst-vaapi commits.

Review of attachment 264830 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapiencoder_h264.c
@@ +201,3 @@
+    profiles[n++] = GST_VAAPI_PROFILE_H264_MAIN;
+  else
+    profiles[n++] = GST_VAAPI_PROFILE_H264_CONSTRAINED_BASELINE;

It is better to dissociate SW and HW profile. SW profile is determined from the coding tools and this is what we will be using to encode the packed headers. HW profile is determined by the HW capabilities.

@@ +206,3 @@
+    case GST_VAAPI_PROFILE_H264_CONSTRAINED_BASELINE:
+      profiles[n++] = GST_VAAPI_PROFILE_H264_BASELINE;
+    case GST_VAAPI_PROFILE_H264_BASELINE:

It is still not correct to fallback from "baseline" to "main" profile.

@@ +448,3 @@
+  constraint_set0_flag = (profile == GST_VAAPI_PROFILE_H264_BASELINE ||
+      profile == GST_VAAPI_PROFILE_H264_CONSTRAINED_BASELINE);
+  constraint_set1_flag = profile == GST_VAAPI_PROFILE_H264_CONSTRAINED_BASELINE;

I don't think this is right. constraint_set0_flag relates to A.2.1 constraints (baseline profile) and constraint_set1_flag relates to A.2.2 constraints (main profile). So, set0_flag = { baseline, constrained-baseline } and set1_flag = { constrained-baseline, main }.

(In reply to comment #18)
> Review of attachment 264830 [details] [review]:
> 
> ::: gst-libs/gst/vaapi/gstvaapiencoder_h264.c
> @@ +201,3 @@
> +    profiles[n++] = GST_VAAPI_PROFILE_H264_MAIN;
> +  else
> +    profiles[n++] = GST_VAAPI_PROFILE_H264_CONSTRAINED_BASELINE;
> 
> It is better to dissociate SW and HW profile. SW profile is determined from the
> coding tools and this is what we will be using to encode the packed headers. HW
> profile is determined by the HW capabilities.
I'm not sure whether it's ok separate SW/HW profile for every hardware driver. e.g Baseline proflie of Redundant slice which may depends on driver itself since libva doesn't have the coding tools. But I'm ok to separate it and GEN/PowerVR can doesn't have special features. And more important, if this can make you happy to accept.

> 
> @@ +206,3 @@
> +    case GST_VAAPI_PROFILE_H264_CONSTRAINED_BASELINE:
> +      profiles[n++] = GST_VAAPI_PROFILE_H264_BASELINE;
> +    case GST_VAAPI_PROFILE_H264_BASELINE:
> 
> It is still not correct to fallback from "baseline" to "main" profile.
I don't think this is similar like decoder. Encoder profile should depends on coding tool settings. we don't have special settings of FMO/ASO/RS, then we can switch to main profile for gst-vaapi's baseline coding tools.

> 
> @@ +448,3 @@
> +  constraint_set0_flag = (profile == GST_VAAPI_PROFILE_H264_BASELINE ||
> +      profile == GST_VAAPI_PROFILE_H264_CONSTRAINED_BASELINE);
> +  constraint_set1_flag = profile ==
> GST_VAAPI_PROFILE_H264_CONSTRAINED_BASELINE;
> 
> I don't think this is right. constraint_set0_flag relates to A.2.1 constraints
> (baseline profile) and constraint_set1_flag relates to A.2.2 constraints (main
> profile). So, set0_flag = { baseline, constrained-baseline } and set1_flag = {
> constrained-baseline, main }.
You are right, I forgot to check main profile. will update it.

Created attachment 266137 [details] [review]
0001-encoder-h264-looking-for-correct-profile-to-set

modified according to your suggestion except baseline fallback to main/high.

commit 2b482e8e4bcae0807e672acb9c309a0e9037773c
Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com>
Date:   Fri Jan 10 17:02:44 2014 +0100

    encoder: h264: fix hardware profile lookup.
    
    Fix lookup for a suitable HW profile, as to be used by the underlying
    hardware, based on heuristics that lead to characterize the SW profile,
    i.e. the one used by the SW level encoding logic.
    
    Also fix constraint_set0_flag (A.2.1) and constraint_set1_flag (A.2.2)
    as they should respectively match the baseline and main profile.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=719827

It worked with the latest commit , but if using the commit 2b482e8e4bcae0807e672acb9c309a0e9037773c, then encoding failed: 

Pipeline is PREROLLING ...
/GstPipeline:pipeline0/GstVideoParse:videoparse0.GstPad:src: caps = video/x-raw, format=(string)I420, width=(int)704, height=(int)576, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, colorimetry=(string)bt601, framerate=(fraction)25/1
/GstPipeline:pipeline0/GstVaapiEncodeH264:vaapiencodeh264-0.GstPad:sink: caps = video/x-raw, format=(string)I420, width=(int)704, height=(int)576, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, colorimetry=(string)bt601, framerate=(fraction)25/1
ERROR: from element /GstPipeline:pipeline0/GstVaapiEncodeH264:vaapiencodeh264-0: GStreamer error: negotiation problem.
Additional debug info:
gstvideoencoder.c(1363): gst_video_encoder_chain (): /GstPipeline:pipeline0/GstVaapiEncodeH264:vaapiencodeh264-0:
encoder not initialized
ERROR: pipeline doesn't want to preroll.
Setting pipeline to NULL ...
/GstPipeline:pipeline0/GstQTMux:qtmux0.GstPad:src: caps = video/quicktime, variant=(string)apple
Freeing pipeline ...

(In reply to comment #22)
> It worked with the latest commit , but if using the commit
> 2b482e8e4bcae0807e672acb9c309a0e9037773c, then encoding failed: 

Yes, that's expected. I moved the default change to "constrained-baseline" profile to the next commit (b59a557). What only matters is git master branch now, there is no point to test back in time. :)

close it