SVC streams proved to be a bit tricky. Following the same method I used in bug 732265 doesn't work and so I try to flag NAL units 14, 15 and 20 with GST_VAAPI_DECODER_UNIT_FLAG_SKIP in gst_vaapi_decoder_h264_parse().
The problem I am facing right now is with the GST_VAAPI_DECODER_UNIT_FLAG_FRAME_END flag.
h264parse uses the whole stream and doesn't "detect" AU start and end correctly, this means that at_au_end is not reliable. For example, in SVC conformance bitstream SVCHMTS-1-L3.264 a sequence of NAL units goes like this (where START means that GST_VAAPI_DECODER_UNIT_FLAG_FRAME_START is set and END means that GST_VAAPI_DECODER_UNIT_FLAG_FRAME_END is set):

1. nalu 14 flags: START=1 END=0     <-- marked to be skipped
2. nalu 05 flags: START=1 END=1
3. nalu 14 flags: START=1 END=0     <-- marked to be skipped
4. nalu 05 flags: START=0 END=0
5. nalu 20 flags: START=0 END=0     <-- marked to be skipped
6. nalu 20 flags: START=0 END=0     <-- marked to be skipped
7. nalu 20 flags: START=0 END=1     <-- marked to be skipped

Now, if I remove the 14, 15, 20 units from the file before executing (a stream that uses the main profile) the same sequence becomes:

1. nalu 5 flags: START=1 END=0
2. nalu 5 flags: START=0 END=1

It's not hard to correct the flags with the following logic (only to be used for SVC streams):

- Mark 14, 15, 20 units as skippable and set their FRAME_START and FRAME_END flags to 0.
- Skip the gst_vaapi_parser_info_h264_replace (&priv->prev_pi, pi) line for units 14, 15, 20 so that prev_pi links to units that aren't going to be skipped (shouldn't this be the default behavior btw?).
- Ignore at_au_end.
- Use is_new_picture() and is_new_access_unit() to detect FRAME_START and then mark the previous slice as FRAME_END with prev_slice_pi.

The problem with this is that the way gstvaapidecoder.c works:

1. FRAME_END only has an effect immediately after gst_vaapi_decoder_h264_parse() is called. If a previous unit is flagged nothing happens.
2. It's not possible to take a peek at the next units (afaik). This is the way h264parse does it and the way I made SVC streams to work for bug 732267: search NAL units in the data stream until you find an Annex-A one and then use gst_h264_parse_collect_nal()'s logic to determine AU endings.

Any ideas?

(In reply to Orestis Floros from comment #1)
> SVC streams proved to be a bit tricky. Following the same method I used in
> bug 732265 doesn't work

Can you explain why?

(In reply to Orestis Floros from comment #1)
> SVC streams proved to be a bit tricky. Following the same method I used in
> bug 732265 doesn't work and so I try to flag NAL units 14, 15 and 20 with
> GST_VAAPI_DECODER_UNIT_FLAG_SKIP in gst_vaapi_decoder_h264_parse().
> The problem I am facing right now is with the
> GST_VAAPI_DECODER_UNIT_FLAG_FRAME_END flag.
> h264parse uses the whole stream and doesn't "detect" AU start and end
> correctly, this means that at_au_end is not reliable. 

If there is an issue with h264parse to correctly detect the AU, why can't we try to fix that issue first(in h264parse)? 
IIUC from your comments, fixing h264parse to make the AU detection correct for SVC will help to make the vaapih264dec patch simple enough like what you did for #732265, right?

(In reply to sreerenj from comment #3)
> (In reply to Orestis Floros from comment #1)
> > SVC streams proved to be a bit tricky. Following the same method I used in
> > bug 732265 doesn't work and so I try to flag NAL units 14, 15 and 20 with
> > GST_VAAPI_DECODER_UNIT_FLAG_SKIP in gst_vaapi_decoder_h264_parse().
> > The problem I am facing right now is with the
> > GST_VAAPI_DECODER_UNIT_FLAG_FRAME_END flag.
> > h264parse uses the whole stream and doesn't "detect" AU start and end
> > correctly, this means that at_au_end is not reliable. 
> 
> If there is an issue with h264parse to correctly detect the AU, why can't we
> try to fix that issue first(in h264parse)? 
> IIUC from your comments, fixing h264parse to make the AU detection correct
> for SVC will help to make the vaapih264dec patch simple enough like what you
> did for #732265, right?

Are we actually sure that h264parse is wrong here? I assumed it was because of the extra units but never thought it could be this. For example, in the sequence 14 05 20 20 20 where AU END is set on the last NAL the detection could be correct for an SVC stream but there is no way for h264parse to know that the downstream element doesn't want the non-Annex-A NALUs.

(In reply to Orestis Floros from comment #4)
> (In reply to sreerenj from comment #3)
> > (In reply to Orestis Floros from comment #1)
> > > SVC streams proved to be a bit tricky. Following the same method I used in
> > > bug 732265 doesn't work and so I try to flag NAL units 14, 15 and 20 with
> > > GST_VAAPI_DECODER_UNIT_FLAG_SKIP in gst_vaapi_decoder_h264_parse().
> > > The problem I am facing right now is with the
> > > GST_VAAPI_DECODER_UNIT_FLAG_FRAME_END flag.
> > > h264parse uses the whole stream and doesn't "detect" AU start and end
> > > correctly, this means that at_au_end is not reliable. 
> > 
> > If there is an issue with h264parse to correctly detect the AU, why can't we
> > try to fix that issue first(in h264parse)? 
> > IIUC from your comments, fixing h264parse to make the AU detection correct
> > for SVC will help to make the vaapih264dec patch simple enough like what you
> > did for #732265, right?
> 
> Are we actually sure that h264parse is wrong here? I assumed it was because
> of the extra units but never thought it could be this. For example, in the
> sequence 14 05 20 20 20 where AU END is set on the last NAL the detection
> could be correct for an SVC stream but there is no way for h264parse to know
> that the downstream element doesn't want the non-Annex-A NALUs.

There could be errors in h264parse too. Even for MVC there were few misinterpretations in frame boundary detection(not sure it is fixed now).
For SVC, there are many missing points. First of all, we haven't added svc parsing support in codecparser library(gst-libs/gst/codecparsers) yet. So what ever header parsing you do in gst/videoparsers/gsth264parse.c is going to fail most likely.

But before going into those, I recommend you to enable the "pass-through" mode in h264parse (so that it won't do any parsing or skipping) and add the base-only support in vaapih264dec(why can't it work like your mvc patch?).

"Sequence 14 05 20 20 20 where the AU END set on last nal" means: A multi-slice encoded stream with different nal types which is wrong I believe. Because if there an IDR slice, all other slices in the same picture should be also IDR.

Just had a quick look into h264parse and yes it is missing svc stuff in many places. 
And for your case(AU detection), SLICE_EXT is not considered in the code which 
 is doing picture_start/complete detection.

(In reply to sreerenj from comment #6)
> Just had a quick look into h264parse and yes it is missing svc stuff in many
> places. 
> And for your case(AU detection), SLICE_EXT is not considered in the code
> which 
>  is doing picture_start/complete detection.

Ok, thanks a lot sreerenj. I'll research this more tomorrow.

What plan of action do you propose for this bug?

(In reply to Orestis Floros from comment #7)
> (In reply to sreerenj from comment #6)
> > Just had a quick look into h264parse and yes it is missing svc stuff in many
> > places. 
> > And for your case(AU detection), SLICE_EXT is not considered in the code
> > which 
> >  is doing picture_start/complete detection.
> 
> Ok, thanks a lot sreerenj. I'll research this more tomorrow.
> 
> What plan of action do you propose for this bug?

Fix it :)

(In reply to sreerenj from comment #2)
> (In reply to Orestis Floros from comment #1)
> > SVC streams proved to be a bit tricky. Following the same method I used in
> > bug 732265 doesn't work
> 
> Can you explain why?

sps->profile_idc in decode_picture() was always 83 or 86 so the is_svc_profile() check always returned TRUE and all frames were dropped. I don't know why, I'll return to that later.

From the spec:

> 7.4.1.2.3 Order of NAL units and coded pictures and association to access units
> ...
> NOTE 1 – Some bitstreams that conform to profiles specified in Annexes G or H may violate the NAL unit order specified in this clause. Conditions under which such a violation of the NAL unit order occurs are specified in clauses G.7.4.1.2.3 and H.7.4.1.2.3.
> ...
> The first of any of the following NAL units after the last VCL NAL unit of a primary coded picture specifies the start of a new access unit:
> - ...
> - NAL units with nal_unit_type in the range of 14 to 18, inclusive (when present),
> - ...

This is done in gst_h264_parse_collect_nal() for h264parse:

complete = h264parse->picture_start && (( ... ) || (nal_type >= 14 && nal_type <= 18));

G.7.4.1.2.3 doesn't mention anything about prefix (nal_type = 14) units other than:

> The following additional constraints shall be obeyed:
> – Each NAL unit with nal_unit_type equal to 1 or 5 shall be immediately preceded by a prefix NAL unit.
> – In bitstreams conforming to this Recommendation | International Standard, each prefix NAL unit shall be immediately followed by a NAL unit with nal_unit_type equal to 1 or 5.

but:

> G.3.42 prefix NAL unit: A NAL unit with nal_unit_type equal to 14 that immediately precedes in decoding order a NAL unit with nal_unit_type equal to 1 or 5. The NAL unit that immediately succeeds the prefix NAL unit in decoding order is referred to as the associated NAL unit. The prefix NAL unit contains data associated with the associated NAL unit, which are considered to be part of the associated NAL unit.
> G.3.67 VCL NAL unit: A collective term for coded slice NAL units and prefix NAL units.

Now, I am not sure if this means that prefix units should be considered part of the associated NAL unit and therefore not be used for AU detection in SVC streams. If I change nal_type >= 14 to nal_type >= 15 in gst_h264_parse_collect_nal() it solves all problems in SVC conformance bitstreams relating to AU detection without playing with FRAME_{START,END} flags in vaapih264dec.

You are almost there :)

For MVC and SVC, Prefix NALs should always come before a base layer slice nal.  H264parse code is not dealing it correctly. 
As you said "complete = xxxx" shouldn't include the prefix-nal checking.
On top of that, the picture_start finding should include the prefix nal and the slice extension nal.

(In reply to sreerenj from comment #11)
> You are almost there :)
> 
> For MVC and SVC, Prefix NALs should always come before a base layer slice
> nal.  H264parse code is not dealing it correctly. 
> As you said "complete = xxxx" shouldn't include the prefix-nal checking.
> On top of that, the picture_start finding should include the prefix nal and
> the slice extension nal.

Actually, we can completely skip prefix nal units when searching for the next nalu, nnalu, in SVC streams. This way we can end the au when first_mb_in_slice == 0 for the associated slice unit.

That said, if h264parse AU detection is faulty for SVC streams shouldn't the same apply to MVC streams as well?

(In reply to Orestis Floros from comment #12)
> (In reply to sreerenj from comment #11)
> > You are almost there :)
> > 
> > For MVC and SVC, Prefix NALs should always come before a base layer slice
> > nal.  H264parse code is not dealing it correctly. 
> > As you said "complete = xxxx" shouldn't include the prefix-nal checking.
> > On top of that, the picture_start finding should include the prefix nal and
> > the slice extension nal.
> 
> Actually, we can completely skip prefix nal units when searching for the
> next nalu, nnalu, in SVC streams. This way we can end the au when
> first_mb_in_slice == 0 for the associated slice unit.

Yup.

> 
> That said, if h264parse AU detection is faulty for SVC streams shouldn't the
> same apply to MVC streams as well?

Yes, it should be broken for MVC too.

Created attachment 357926 [details] [review]
h264parse: skip prefix NALs for AU detection

These are my changes so far for h264parse that we discussed with Sreerenj. This should be placed after attachment 356212 [details] [review] from bug 732267.

(In reply to sreerenj from comment #11)
> On top of that, the picture_start finding should include the prefix nal and
> the slice extension nal.

Adding slice extension to complete = ... causes multiple errors for MVC streams so I am not sure about that yet:

> vaapi gstvaapidecoder_h264.c:2923:exec_picture_refs_modification_1: list 0 entry 0 is empty

Created attachment 357927 [details] [review]
vaapidecode: force add h264 SVC profiles in caps

Created attachment 357928 [details] [review]
libs: decoder: h264: decode SVC base view only

Created attachment 357929 [details] [review]
libs: decoder: h264: remove other flags from skipped units

With these patches the only conformance bitstreams that still have problems are:

SVCHS-2.264
SVCHS-2-L1.264
SVCHST-3.264
SVCHST-3-L1.264
SVCHST-4.264
SVCHST-4-L1.264

(In reply to Orestis Floros from comment #14)
> Created attachment 357926 [details] [review] [review]
> h264parse: skip prefix NALs for AU detection
> 
> These are my changes so far for h264parse that we discussed with Sreerenj.
> This should be placed after attachment 356212 [details] [review] [review] from bug
> 732267.
> 
> (In reply to sreerenj from comment #11)
> > On top of that, the picture_start finding should include the prefix nal and
> > the slice extension nal.
> 
> Adding slice extension to complete = ... causes multiple errors for MVC
> streams so I am not sure about that yet:
> 
> > vaapi gstvaapidecoder_h264.c:2923:exec_picture_refs_modification_1: list 0 entry 0 is empty

Please show me the code where you added the check? Also, Which video sample?

Created attachment 357966 [details] [review]
h264parse: add coded slice extension in AU detection

(In reply to sreerenj from comment #19)
> (In reply to Orestis Floros from comment #14)
> > Created attachment 357926 [details] [review] [review] [review]
> > h264parse: skip prefix NALs for AU detection
> > 
> > These are my changes so far for h264parse that we discussed with Sreerenj.
> > This should be placed after attachment 356212 [details] [review] [review] [review] from bug
> > 732267.
> > 
> > (In reply to sreerenj from comment #11)
> > > On top of that, the picture_start finding should include the prefix nal and
> > > the slice extension nal.
> > 
> > Adding slice extension to complete = ... causes multiple errors for MVC
> > streams so I am not sure about that yet:
> > 
> > > vaapi gstvaapidecoder_h264.c:2923:exec_picture_refs_modification_1: list 0 entry 0 is empty
> 
> Please show me the code where you added the check? Also, Which video sample?

For most samples:

MVCDS3.264
MVCDS-4.264
MVCDS-4_old.264
MVCDS-5.264
MVCDS-5_old.264
MVCDS-6.264
MVCDS-6_old.264
MVCICT-1.264
MVCICT-2.264
MVCNV1.264
MVCNV-2.264
MVCNV-2_old.264
MVCNV-3.264
MVCNV-3_old.264
MVCNV4.264
MVCRP_3.264
MVCRP_4.264
MVCRP_5.264
MVCRP_6.264
MVCSPS-1.264
MVCSPS-1_old.264
MVCSPS-2.264
MVCSPS-2_old.264

from ITU-T_H.264.1(2016-02)_MVC_bitstreams.zip from http://www.itu.int/net/itu-t/sigdb/spevideo/VideoForm-s.aspx?val=102002641

One access unit can have multiple view components, and yes you don't have to add the slice_ext in the code for checking the complete_au. Sorry about the confusion.
But ideally, we should add other checkings like "compare the view_ids of nals" for vcl boundary detection (even though current code can work without that).


BTW I hit a segfault issue while testing your branch:

Generate an mvc sample like this: 
gst-launch-1.0 -v videotestsrc num-buffers=30 blocksize=115200 ! vaapih264enc num-views=2 ! video/x-h264, profile=stereo-high ! filesink location=sample.mvc

playback the stream with only base view
gst-launch-1.0 filesrc location=sample.mvc ! h264parse ! vaapih264dec base-only=true ! vaapisink

Created attachment 358028 [details] [review]
(In reply to sreerenj from comment #21)

> One access unit can have multiple view components, and yes you don't have to
> add the slice_ext in the code for checking the complete_au. Sorry about the
> confusion.
> But ideally, we should add other checkings like "compare the view_ids of
> nals" for vcl boundary detection (even though current code can work without
> that).

I think this requires the previous slice / vlc nalu right? I think h264parse doesn't keep that.

> BTW I hit a segfault issue while testing your branch:
> 
> Generate an mvc sample like this: 
> gst-launch-1.0 -v videotestsrc num-buffers=30 blocksize=115200 !
> vaapih264enc num-views=2 ! video/x-h264, profile=stereo-high ! filesink
> location=sample.mvc
> 
> playback the stream with only base view
> gst-launch-1.0 filesrc location=sample.mvc ! h264parse ! vaapih264dec
> base-only=true ! vaapisink

Thanks, fixed.

(In reply to Orestis Floros from comment #22)
> Created attachment 358028 [details] [review] [review]
> (In reply to sreerenj from comment #21)
> 
> > One access unit can have multiple view components, and yes you don't have to
> > add the slice_ext in the code for checking the complete_au. Sorry about the
> > confusion.
> > But ideally, we should add other checkings like "compare the view_ids of
> > nals" for vcl boundary detection (even though current code can work without
> > that).
> 
> I think this requires the previous slice / vlc nalu right? I think h264parse
> doesn't keep that.

Yup, It requires some extra parsing code. But no need to include that one here I believe (unless it is necessary to fix any known issue). 

> 
> > BTW I hit a segfault issue while testing your branch:
> > 
> > Generate an mvc sample like this: 
> > gst-launch-1.0 -v videotestsrc num-buffers=30 blocksize=115200 !
> > vaapih264enc num-views=2 ! video/x-h264, profile=stereo-high ! filesink
> > location=sample.mvc
> > 
> > playback the stream with only base view
> > gst-launch-1.0 filesrc location=sample.mvc ! h264parse ! vaapih264dec
> > base-only=true ! vaapisink
> 
> Thanks, fixed.

K great.
So I guess the only know issue is https://bugzilla.gnome.org/show_bug.cgi?id=732266#c18 . right?

(In reply to sreerenj from comment #23)
> (In reply to Orestis Floros from comment #22)
> > Created attachment 358028 [details] [review] [review] [review]
> > (In reply to sreerenj from comment #21)
> > 
> > > One access unit can have multiple view components, and yes you don't have to
> > > add the slice_ext in the code for checking the complete_au. Sorry about the
> > > confusion.
> > > But ideally, we should add other checkings like "compare the view_ids of
> > > nals" for vcl boundary detection (even though current code can work without
> > > that).
> > 
> > I think this requires the previous slice / vlc nalu right? I think h264parse
> > doesn't keep that.
> 
> Yup, It requires some extra parsing code. But no need to include that one
> here I believe (unless it is necessary to fix any known issue). 
> 
> > 
> > > BTW I hit a segfault issue while testing your branch:
> > > 
> > > Generate an mvc sample like this: 
> > > gst-launch-1.0 -v videotestsrc num-buffers=30 blocksize=115200 !
> > > vaapih264enc num-views=2 ! video/x-h264, profile=stereo-high ! filesink
> > > location=sample.mvc
> > > 
> > > playback the stream with only base view
> > > gst-launch-1.0 filesrc location=sample.mvc ! h264parse ! vaapih264dec
> > > base-only=true ! vaapisink
> > 
> > Thanks, fixed.
> 
> K great.
> So I guess the only know issue is
> https://bugzilla.gnome.org/show_bug.cgi?id=732266#c18 . right?

yes.

The errors:

SVCHS-2.264:
lt-gst-launch-1.0: gen75_mfd.c:556: gen75_mfd_avc_img_state: Assertion `pic_param->pic_fields.bits.field_pic_flag == 0' failed.

SVCHS-2-L1.264:
lt-gst-launch-1.0: gen75_mfd.c:556: gen75_mfd_avc_img_state: Assertion `pic_param->pic_fields.bits.field_pic_flag == 0' failed.

SVCHST-3.264:
vaapi gstvaapidecoder_h264.c:2752:find_long_term_reference: found no long-term reference picture with LongTermPicNum = 0
vaapi gstvaapidecoder_h264.c:2752:find_long_term_reference: found no long-term reference picture with LongTermPicNum = 0
vaapi gstvaapidecoder_h264.c:2752:find_long_term_reference: found no long-term reference picture with LongTermPicNum = 0
vaapi gstvaapidecoder_h264.c:2752:find_long_term_reference: found no long-term reference picture with LongTermPicNum = 0
vaapi gstvaapidecoder_h264.c:2752:find_long_term_reference: found no long-term reference picture with LongTermPicNum = 0
vaapi gstvaapidecoder_h264.c:2752:find_long_term_reference: found no long-term reference picture with LongTermPicNum = 0

SVCHST-3-L1.264:
vaapi gstvaapidecoder_h264.c:2752:find_long_term_reference: found no long-term reference picture with LongTermPicNum = 0
vaapi gstvaapidecoder_h264.c:2752:find_long_term_reference: found no long-term reference picture with LongTermPicNum = 0
vaapi gstvaapidecoder_h264.c:2752:find_long_term_reference: found no long-term reference picture with LongTermPicNum = 0
vaapi gstvaapidecoder_h264.c:2752:find_long_term_reference: found no long-term reference picture with LongTermPicNum = 0
vaapi gstvaapidecoder_h264.c:2752:find_long_term_reference: found no long-term reference picture with LongTermPicNum = 0
vaapi gstvaapidecoder_h264.c:2752:find_long_term_reference: found no long-term reference picture with LongTermPicNum = 0

SVCHST-4.264 and SVCHST-4-L1.264 don't have any errors in the log but the video is still wrong.

(In reply to Orestis Floros from comment #24)
> (In reply to sreerenj from comment #23)
> > (In reply to Orestis Floros from comment #22)
> > > Created attachment 358028 [details] [review] [review] [review] [review]
> > > (In reply to sreerenj from comment #21)
> > > 
> > > > One access unit can have multiple view components, and yes you don't have to
> > > > add the slice_ext in the code for checking the complete_au. Sorry about the
> > > > confusion.
> > > > But ideally, we should add other checkings like "compare the view_ids of
> > > > nals" for vcl boundary detection (even though current code can work without
> > > > that).
> > > 
> > > I think this requires the previous slice / vlc nalu right? I think h264parse
> > > doesn't keep that.
> > 
> > Yup, It requires some extra parsing code. But no need to include that one
> > here I believe (unless it is necessary to fix any known issue). 
> > 
> > > 
> > > > BTW I hit a segfault issue while testing your branch:
> > > > 
> > > > Generate an mvc sample like this: 
> > > > gst-launch-1.0 -v videotestsrc num-buffers=30 blocksize=115200 !
> > > > vaapih264enc num-views=2 ! video/x-h264, profile=stereo-high ! filesink
> > > > location=sample.mvc
> > > > 
> > > > playback the stream with only base view
> > > > gst-launch-1.0 filesrc location=sample.mvc ! h264parse ! vaapih264dec
> > > > base-only=true ! vaapisink
> > > 
> > > Thanks, fixed.
> > 
> > K great.
> > So I guess the only know issue is
> > https://bugzilla.gnome.org/show_bug.cgi?id=732266#c18 . right?
> 
> yes.
> 
> The errors:
> 
> SVCHS-2.264:
> lt-gst-launch-1.0: gen75_mfd.c:556: gen75_mfd_avc_img_state: Assertion
> `pic_param->pic_fields.bits.field_pic_flag == 0' failed.
> 
> SVCHS-2-L1.264:
> lt-gst-launch-1.0: gen75_mfd.c:556: gen75_mfd_avc_img_state: Assertion
> `pic_param->pic_fields.bits.field_pic_flag == 0' failed.
> 

This is the driver assertion.
Because somehow middleware set the frame_mbs_only_flag in pic_params (which means not an interlaced content) but set the filed_pic_flag (which means frame has interlaced fields) in slice_header.
Check the parsed values, could be a wrongly encoded stream.

Created attachment 358039 [details] [review]
libs: decoder: h264: remove other flags from skipped units

Created attachment 358041 [details] [review]
libs: decoder: h264: decode SVC base view only

This should fix everything.

Review of attachment 357927 [details] [review]:

The patch lgtm.

Just for a note:
There could be more refinement possible, for eg: even if the underlined driver doesn't have support for high-profile(but has support for main or constrained-base-line), it can still support the svc/mvc stream decode(base only) where the base layer/view is encoded as constrained-base-line or main profile. But this is complicated since it requires more signaling to the upstream since we can't advertise the mvc/svc support in sink caps just based on the hardware capability to decode the constrained-base-line or main.

Review of attachment 358039 [details] [review]:

Can you explain "why it requires"  in the commit message? 

Becuase there are other parsed units which were set for skipping(AU_DELIMITER, FILLTER_DATA) but still we count them for finding frame_start/au_start.

(In reply to sreerenj from comment #29)
> Review of attachment 358039 [details] [review] [review]:
> 
> Can you explain "why it requires"  in the commit message? 
> 
> Becuase there are other parsed units which were set for
> skipping(AU_DELIMITER, FILLTER_DATA) but still we count them for finding
> frame_start/au_start.

Actually, It's enough to only remove the START and END flags from PREFIX NALs and leave everything else the same.

(In reply to Orestis Floros from comment #30)
> (In reply to sreerenj from comment #29)
> > Review of attachment 358039 [details] [review] [review] [review]:
> > 
> > Can you explain "why it requires"  in the commit message? 
> > 
> > Becuase there are other parsed units which were set for
> > skipping(AU_DELIMITER, FILLTER_DATA) but still we count them for finding
> > frame_start/au_start.
> 
> Actually, It's enough to only remove the START and END flags from PREFIX
> NALs and leave everything else the same.

*AU_START and FRAME_START not END

(In reply to Orestis Floros from comment #31)
> (In reply to Orestis Floros from comment #30)
> > (In reply to sreerenj from comment #29)
> > > Review of attachment 358039 [details] [review] [review] [review] [review]:
> > > 
> > > Can you explain "why it requires"  in the commit message? 
> > > 
> > > Becuase there are other parsed units which were set for
> > > skipping(AU_DELIMITER, FILLTER_DATA) but still we count them for finding
> > > frame_start/au_start.
> > 
> > Actually, It's enough to only remove the START and END flags from PREFIX
> > NALs and leave everything else the same.
> 
> *AU_START and FRAME_START not END

But then it messes up this stream:

gst-launch-1.0 filesrc location=SVCHS-2.264 ! h264parse ! h264parse ! video/x-h264, stream-format=byte-stream, alignment=nal ! vaapih264dec ! vaapisink

Review of attachment 358041 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c
@@ +1793,3 @@
 
+  if (priv->base_only && !GST_H264_IS_MVC_NALU (&pi->nalu))
+    result = gst_h264_parse_sps (&pi->nalu, sps, FALSE);

Why does it only for SVC? svc and mvc base-only mode should behave similarly. right?

@@ +1941,3 @@
   GST_DEBUG ("decode subset SPS");
 
+  /* Don't replace. Could overwrite needeed SPS's. */

Bit confused with this comment. I understood that in base-only case, we don't have to store the subset_sps. But when can this "Overwrite needed sps" happen?

Created attachment 358103 [details] [review]
libs: decoder: h264: decode SVC base view only

(In reply to sreerenj from comment #33)
> Review of attachment 358041 [details] [review] [review]:
>
> ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c
> @@ +1793,3 @@
>
> +  if (priv->base_only && !GST_H264_IS_MVC_NALU (&pi->nalu))
> +    result = gst_h264_parse_sps (&pi->nalu, sps, FALSE);
>
> Why does it only for SVC? svc and mvc base-only mode should behave
> similarly. right?

I don't know how I missed it, this part is buggy and breaks MVC streams with base-only=TRUE right now. This patch should do it.

I don't know why they behave differently here, I made the distinction through trial and error.

> @@ +1941,3 @@
>    GST_DEBUG ("decode subset SPS");
>
> +  /* Don't replace. Could overwrite needeed SPS's. */
>
> Bit confused with this comment. I understood that in base-only case, we
> don't have to store the subset_sps. But when can this "Overwrite needed sps"
> happen?

spec says:

> NOTE 7 ? A decoder must be capable of simultaneously storing the contents of the sequence parameter sets and subset sequence
parameter sets for all values of seq_parameter_set_id. The content of the sequence parameter set with a particular value of
seq_parameter_set_id is overwritten when a new sequence parameter set NAL unit with the same value of seq_parameter_set_id is
received, and the content of the subset sequence parameter set with a particular value of seq_parameter_set_id is overwritten when
a new subset sequence parameter set NAL unit with the same value of seq_parameter_set_id is received.

Eg in SVCHS-2.264:
nalu type=7 has seq_parameter_set_id=0 and next nalu type=15 has seq_parameter_set_id=0.

(In reply to Orestis Floros from comment #34)
> Created attachment 358103 [details] [review] [review]
> libs: decoder: h264: decode SVC base view only
> 
> (In reply to sreerenj from comment #33)
> > Review of attachment 358041 [details] [review] [review] [review]:
> >
> > ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c
> > @@ +1793,3 @@
> >
> > +  if (priv->base_only && !GST_H264_IS_MVC_NALU (&pi->nalu))
> > +    result = gst_h264_parse_sps (&pi->nalu, sps, FALSE);
> >
> > Why does it only for SVC? svc and mvc base-only mode should behave
> > similarly. right?
> 
> I don't know how I missed it, this part is buggy and breaks MVC streams with
> base-only=TRUE right now. This patch should do it.
> 
> I don't know why they behave differently here, I made the distinction
> through trial and error.

Hm, we need an explanation other than trial-and-error-fix-this.

(In reply to Orestis Floros from comment #34)
> Created attachment 358103 [details] [review] [review]
> libs: decoder: h264: decode SVC base view only
> 
> (In reply to sreerenj from comment #33)
> > Review of attachment 358041 [details] [review] [review] [review]:

> 
> > @@ +1941,3 @@
> >    GST_DEBUG ("decode subset SPS");
> >
> > +  /* Don't replace. Could overwrite needeed SPS's. */
> >
> > Bit confused with this comment. I understood that in base-only case, we
> > don't have to store the subset_sps. But when can this "Overwrite needed sps"
> > happen?
> 
> spec says:
> 
> > NOTE 7 ? A decoder must be capable of simultaneously storing the contents of the sequence parameter sets and subset sequence
> parameter sets for all values of seq_parameter_set_id. The content of the
> sequence parameter set with a particular value of
> seq_parameter_set_id is overwritten when a new sequence parameter set NAL
> unit with the same value of seq_parameter_set_id is
> received, and the content of the subset sequence parameter set with a
> particular value of seq_parameter_set_id is overwritten when
> a new subset sequence parameter set NAL unit with the same value of
> seq_parameter_set_id is received.
> 

Here the spec clearly saying that "SPS should be replaced by another SPS with the same id" and "SUBSET_SPS should be replaced by another SUBSET_SPS with the same id". 

Am I misinterpreting something here? :)



> Eg in SVCHS-2.264:
> nalu type=7 has seq_parameter_set_id=0 and next nalu type=15 has
> seq_parameter_set_id=0.


Hm, I am afraid we need some serious changes in codecparser/videoparser/decoders since the GstH264NalParser is not keeping separate array for sps and subset sps.

Is it the only sample which exhibiting this behavior?

(In reply to sreerenj from comment #36)
> Here the spec clearly saying that "SPS should be replaced by another SPS
> with the same id" and "SUBSET_SPS should be replaced by another SUBSET_SPS
> with the same id".
>
> Am I misinterpreting something here? :)

I think we don't disagree here since you say:

> Hm, I am afraid we need some serious changes in
> codecparser/videoparser/decoders since the GstH264NalParser is not keeping
> separate array for sps and subset sps.

Yes, my solution is a bit hacky.

> Is it the only sample which exhibiting this behavior?

With some quick scripting I found that 137 out of 258 SVC *.264 conformance bitstreams exhibit this behavior but 0 out of 34 of the MVC ones.

(In reply to sreerenj from comment #35)
> (In reply to Orestis Floros from comment #34)
> > Created attachment 358103 [details] [review] [review] [review]
> > libs: decoder: h264: decode SVC base view only
> > 
> > (In reply to sreerenj from comment #33)
> > > Review of attachment 358041 [details] [review] [review] [review] [review]:
> > >
> > > ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c
> > > @@ +1793,3 @@
> > >
> > > +  if (priv->base_only && !GST_H264_IS_MVC_NALU (&pi->nalu))
> > > +    result = gst_h264_parse_sps (&pi->nalu, sps, FALSE);
> > >
> > > Why does it only for SVC? svc and mvc base-only mode should behave
> > > similarly. right?
> > 
> > I don't know how I missed it, this part is buggy and breaks MVC streams with
> > base-only=TRUE right now. This patch should do it.
> > 
> > I don't know why they behave differently here, I made the distinction
> > through trial and error.
> 
> Hm, we need an explanation other than trial-and-error-fix-this.

I think it is related to nalparser. I'd bet that it has something to do with lack of SVC support in codecparsers/gsth264parser.c. This was the change that fixed the issue from:

(In reply to sreerenj from comment #25)
> This is the driver assertion.
> Because somehow middleware set the frame_mbs_only_flag in pic_params (which
> means not an interlaced content) but set the filed_pic_flag (which means
> frame has interlaced fields) in slice_header.
> Check the parsed values, could be a wrongly encoded stream.

I'll try to find a better explanation.

(In reply to Orestis Floros from comment #37)
> (In reply to sreerenj from comment #36)
> > Here the spec clearly saying that "SPS should be replaced by another SPS
> > with the same id" and "SUBSET_SPS should be replaced by another SUBSET_SPS
> > with the same id".
> >
> > Am I misinterpreting something here? :)
> 
> I think we don't disagree here since you say:
> 
> > Hm, I am afraid we need some serious changes in
> > codecparser/videoparser/decoders since the GstH264NalParser is not keeping
> > separate array for sps and subset sps.
> 
> Yes, my solution is a bit hacky.
> 
> > Is it the only sample which exhibiting this behavior?
> 
> With some quick scripting I found that 137 out of 258 SVC *.264 conformance
> bitstreams exhibit this behavior but 0 out of 34 of the MVC ones.

Anyway changing nal_parser with a separate array for subset_sps will shake the implementation here and there.
For the base-only scenario, I am fine with your patch which is avoiding the override.

(In reply to Orestis Floros from comment #38)
> (In reply to sreerenj from comment #35)
> > (In reply to Orestis Floros from comment #34)
> > > Created attachment 358103 [details] [review] [review] [review] [review]
> > > libs: decoder: h264: decode SVC base view only
> > > 
> > > (In reply to sreerenj from comment #33)
> > > > Review of attachment 358041 [details] [review] [review] [review] [review] [review]:
> > > >
> > > > ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c
> > > > @@ +1793,3 @@
> > > >
> > > > +  if (priv->base_only && !GST_H264_IS_MVC_NALU (&pi->nalu))
> > > > +    result = gst_h264_parse_sps (&pi->nalu, sps, FALSE);
> > > >
> > > > Why does it only for SVC? svc and mvc base-only mode should behave
> > > > similarly. right?
> > > 
> > > I don't know how I missed it, this part is buggy and breaks MVC streams with
> > > base-only=TRUE right now. This patch should do it.
> > > 
> > > I don't know why they behave differently here, I made the distinction
> > > through trial and error.
> > 
> > Hm, we need an explanation other than trial-and-error-fix-this.
> 
> I think it is related to nalparser. I'd bet that it has something to do with
> lack of SVC support in codecparsers/gsth264parser.c. This was the change
> that fixed the issue from:

Can you investigate for the real issue?

> (In reply to sreerenj from comment #25)
> > This is the driver assertion.
> > Because somehow middleware set the frame_mbs_only_flag in pic_params (which
> > means not an interlaced content) but set the filed_pic_flag (which means
> > frame has interlaced fields) in slice_header.
> > Check the parsed values, could be a wrongly encoded stream.
> 
> I'll try to find a better explanation.
I thought this has been fixed with your recent patches, isn't it?

(In reply to sreerenj from comment #40)
> (In reply to Orestis Floros from comment #38)
> > (In reply to sreerenj from comment #35)
> > > (In reply to Orestis Floros from comment #34)
> > > > Created attachment 358103 [details] [review] [review] [review] [review] [review]
> > > > libs: decoder: h264: decode SVC base view only
> > > > 
> > > > (In reply to sreerenj from comment #33)
> > > > > Review of attachment 358041 [details] [review] [review] [review] [review] [review] [review]:
> > > > >
> > > > > ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c
> > > > > @@ +1793,3 @@
> > > > >
> > > > > +  if (priv->base_only && !GST_H264_IS_MVC_NALU (&pi->nalu))
> > > > > +    result = gst_h264_parse_sps (&pi->nalu, sps, FALSE);
> > > > >
> > > > > Why does it only for SVC? svc and mvc base-only mode should behave
> > > > > similarly. right?
> > > > 
> > > > I don't know how I missed it, this part is buggy and breaks MVC streams with
> > > > base-only=TRUE right now. This patch should do it.
> > > > 
> > > > I don't know why they behave differently here, I made the distinction
> > > > through trial and error.
> > > 
> > > Hm, we need an explanation other than trial-and-error-fix-this.
> > 
> > I think it is related to nalparser. I'd bet that it has something to do with
> > lack of SVC support in codecparsers/gsth264parser.c. This was the change
> > that fixed the issue from:
> 
> Can you investigate for the real issue?
> 
> > (In reply to sreerenj from comment #25)
> > > This is the driver assertion.
> > > Because somehow middleware set the frame_mbs_only_flag in pic_params (which
> > > means not an interlaced content) but set the filed_pic_flag (which means
> > > frame has interlaced fields) in slice_header.
> > > Check the parsed values, could be a wrongly encoded stream.
> > 
> > I'll try to find a better explanation.
> I thought this has been fixed with your recent patches, isn't it?

I am sorry, my formatting wasn't the best. What I was saying is that I will investigate for the real issue as you say and I mentioned that this change was needed because it fixed the issue from comment #25. I wasn't replying to it, I was just mentioning it.

It should be read like this:

I think it is related to nalparser. I'd bet that it has something to do with lack of SVC support in codecparsers/gsth264parser.c. This was the change that fixed the issue from comment #25.

I'll try to find a better explanation.

(In reply to sreerenj from comment #40)
> (In reply to Orestis Floros from comment #38)
> > (In reply to sreerenj from comment #35)
> > > (In reply to Orestis Floros from comment #34)
> > > > Created attachment 358103 [details] [review] [review] [review] [review] [review]
> > > > libs: decoder: h264: decode SVC base view only
> > > >
> > > > (In reply to sreerenj from comment #33)
> > > > > Review of attachment 358041 [details] [review] [review] [review] [review] [review] [review]:
> > > > >
> > > > > ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c
> > > > > @@ +1793,3 @@
> > > > >
> > > > > +  if (priv->base_only && !GST_H264_IS_MVC_NALU (&pi->nalu))
> > > > > +    result = gst_h264_parse_sps (&pi->nalu, sps, FALSE);
> > > > >
> > > > > Why does it only for SVC? svc and mvc base-only mode should behave
> > > > > similarly. right?
> > > >
> > > > I don't know how I missed it, this part is buggy and breaks MVC streams with
> > > > base-only=TRUE right now. This patch should do it.
> > > >
> > > > I don't know why they behave differently here, I made the distinction
> > > > through trial and error.
> > >
> > > Hm, we need an explanation other than trial-and-error-fix-this.
> >
> > I think it is related to nalparser. I'd bet that it has something to do with
> > lack of SVC support in codecparsers/gsth264parser.c. This was the change
> > that fixed the issue from:
>
> Can you investigate for the real issue?

Why SVCHS-2.264 fails with
"lt-gst-launch-1.0: gen75_mfd.c:556: gen75_mfd_avc_img_state: Assertion `pic_param->pic_fields.bits.field_pic_flag == 0' failed"
when this check isn't enabled:

The stream has one sps and one subset sps unit with the following frame_mbs_only_flag values:

- sps: frame_mbs_only_flag=1
- subset: frame_mbs_only_flag=0

according to 7.3.3 - Slice header syntax:

if( !frame_mbs_only_flag ) {
    field_pic_flag
    if( field_pic_flag )
        bottom_field_flag
}


For the annex-A slices codecparsers/gsth264parser.c:2092 uses the subset sps while it should have used the normal sps, thus !frame_mbs_only_flag == 1 and field_pic_flag gets read.


I don't know why this causes problems for MVC streams:

diff --git a/gst-libs/gst/vaapi/gstvaapidecoder_h264.c b/gst-libs/gst/vaapi/gstvaapidecoder_h264.c
index e727a652..47482c3d 100644
--- a/gst-libs/gst/vaapi/gstvaapidecoder_h264.c
+++ b/gst-libs/gst/vaapi/gstvaapidecoder_h264.c
@@ -1793,7 +1793,7 @@ parse_subset_sps (GstVaapiDecoderH264 * decoder, GstVaapiDecoderUnit * unit)
 
   if (priv->base_only)
     result = gst_h264_parse_sps (&pi->nalu, sps, FALSE);
-  if (!priv->base_only || !is_svc_profile (sps->profile_idc))
+  else
     result =
         gst_h264_parser_parse_subset_sps (priv->parser, &pi->nalu, sps, TRUE);
   if (result != GST_H264_PARSER_OK)


For example with MVCDS3.264 the pipeline freezes (at least on my end) and I don't know how to debug it.

(In reply to Orestis Floros from comment #42)
> (In reply to sreerenj from comment #40)
> > (In reply to Orestis Floros from comment #38)
> > > (In reply to sreerenj from comment #35)
> > > > (In reply to Orestis Floros from comment #34)
> > > > > Created attachment 358103 [details] [review] [review] [review] [review] [review] [review]
> > > > > libs: decoder: h264: decode SVC base view only
> > > > >
> > > > > (In reply to sreerenj from comment #33)
> > > > > > Review of attachment 358041 [details] [review] [review] [review] [review] [review] [review] [review]:
> > > > > >
> > > > > > ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c
> > > > > > @@ +1793,3 @@
> > > > > >
> > > > > > +  if (priv->base_only && !GST_H264_IS_MVC_NALU (&pi->nalu))
> > > > > > +    result = gst_h264_parse_sps (&pi->nalu, sps, FALSE);
> > > > > >
> > > > > > Why does it only for SVC? svc and mvc base-only mode should behave
> > > > > > similarly. right?
> > > > >
> > > > > I don't know how I missed it, this part is buggy and breaks MVC streams with
> > > > > base-only=TRUE right now. This patch should do it.
> > > > >
> > > > > I don't know why they behave differently here, I made the distinction
> > > > > through trial and error.
> > > >
> > > > Hm, we need an explanation other than trial-and-error-fix-this.
> > >
> > > I think it is related to nalparser. I'd bet that it has something to do with
> > > lack of SVC support in codecparsers/gsth264parser.c. This was the change
> > > that fixed the issue from:
> >
> > Can you investigate for the real issue?
> 
> Why SVCHS-2.264 fails with
> "lt-gst-launch-1.0: gen75_mfd.c:556: gen75_mfd_avc_img_state: Assertion
> `pic_param->pic_fields.bits.field_pic_flag == 0' failed"
> when this check isn't enabled:
> 
> The stream has one sps and one subset sps unit with the following
> frame_mbs_only_flag values:
> 
> - sps: frame_mbs_only_flag=1
> - subset: frame_mbs_only_flag=0
> 
> according to 7.3.3 - Slice header syntax:
> 
> if( !frame_mbs_only_flag ) {
>     field_pic_flag
>     if( field_pic_flag )
>         bottom_field_flag
> }
> 
> 
> For the annex-A slices codecparsers/gsth264parser.c:2092 uses the subset sps
> while it should have used the normal sps, thus !frame_mbs_only_flag == 1 and
> field_pic_flag gets read
> 



If we already removed subset_sps and considering only base layer, none of the slice header should reference to subset_sps.
Here the non-base view having field macroblocks.
If the slice heder in base view still reference to the non-base view nals (subset-sps or pps) , there could be two possible reasons:

1: nalparser override the sps with subset sps since both have the same id.
Or
2: stream is wrongly encoded

(In reply to sreerenj from comment #43)
> If we already removed subset_sps and considering only base layer, none of
> the slice header should reference to subset_sps.

But we don't remove subset sps's before parsing them, without the check h264parser's nalparser is updated.

> Here the non-base view having field macroblocks.
> If the slice heder in base view still reference to the non-base view nals
> (subset-sps or pps) , there could be two possible reasons:
> 
> 1: nalparser override the sps with subset sps since both have the same id.
> Or
> 2: stream is wrongly encoded

Yes, (1.) is true, forgot to mention it.

(In reply to Orestis Floros from comment #44)
> (In reply to sreerenj from comment #43)
> > If we already removed subset_sps and considering only base layer, none of
> > the slice header should reference to subset_sps.
> 
> But we don't remove subset sps's before parsing them, without the check
> h264parser's nalparser is updated.


IIRC, you were handling this stuff in your h264parse by invoking the parse api without nalparser.  Why can the same be utilized here(if you have to parse the subset_sps)?


> 
> > Here the non-base view having field macroblocks.
> > If the slice heder in base view still reference to the non-base view nals
> > (subset-sps or pps) , there could be two possible reasons:
> > 
> > 1: nalparser override the sps with subset sps since both have the same id.
> > Or
> > 2: stream is wrongly encoded
> 
> Yes, (1.) is true, forgot to mention it.
Then the ideal fix should go into h264parse I believe :)

(In reply to sreerenj from comment #45)
> (In reply to Orestis Floros from comment #44)
> > (In reply to sreerenj from comment #43)
> > > If we already removed subset_sps and considering only base layer, none of
> > > the slice header should reference to subset_sps.
> > 
> > But we don't remove subset sps's before parsing them, without the check
> > h264parser's nalparser is updated.
> 
> 
> IIRC, you were handling this stuff in your h264parse by invoking the parse
> api without nalparser.  Why can the same be utilized here(if you have to
> parse the subset_sps)?

Yes, this is what I am doing here too. For SVC subset sps I only call gst_h264_parse_sps() that doesn't use the nalparser. The problem is (from comment #33) that I don't do that for MVC streams because they don't play nicely with this for some reason (as I mention in comment #42).

> > > Here the non-base view having field macroblocks.
> > > If the slice heder in base view still reference to the non-base view nals
> > > (subset-sps or pps) , there could be two possible reasons:
> > > 
> > > 1: nalparser override the sps with subset sps since both have the same id.
> > > Or
> > > 2: stream is wrongly encoded
> > 
> > Yes, (1.) is true, forgot to mention it.
> Then the ideal fix should go into h264parse I believe :)

This boils down to what you said in comment #39, right?

(In reply to Orestis Floros from comment #46)
> (In reply to sreerenj from comment #45)
> > (In reply to Orestis Floros from comment #44)
> > > (In reply to sreerenj from comment #43)
> > > > If we already removed subset_sps and considering only base layer, none of
> > > > the slice header should reference to subset_sps.
> > > 
> > > But we don't remove subset sps's before parsing them, without the check
> > > h264parser's nalparser is updated.
> > 
> > 
> > IIRC, you were handling this stuff in your h264parse by invoking the parse
> > api without nalparser.  Why can the same be utilized here(if you have to
> > parse the subset_sps)?
> 
> Yes, this is what I am doing here too. For SVC subset sps I only call
> gst_h264_parse_sps() that doesn't use the nalparser. The problem is (from
> comment #33) that I don't do that for MVC streams because they don't play
> nicely with this for some reason (as I mention in comment #42).
> 
Make sure  the code never using mvc parsed data somewhere.

> 
> > > > Here the non-base view having field macroblocks.
> > > > If the slice heder in base view still reference to the non-base view nals
> > > > (subset-sps or pps) , there could be two possible reasons:
> > > > 
> > > > 1: nalparser override the sps with subset sps since both have the same id.
> > > > Or
> > > > 2: stream is wrongly encoded
> > > 
> > > Yes, (1.) is true, forgot to mention it.
> > Then the ideal fix should go into h264parse I believe :)
> 
> This boils down to what you said in comment #39, right?

Yes. But would be great if you can file a separate bug explaining the reason for required changes in codecparser. Patch to fix this would be nice too :)

(In reply to sreerenj from comment #48)
> > 
> > > > > Here the non-base view having field macroblocks.
> > > > > If the slice heder in base view still reference to the non-base view nals
> > > > > (subset-sps or pps) , there could be two possible reasons:
> > > > > 
> > > > > 1: nalparser override the sps with subset sps since both have the same id.
> > > > > Or
> > > > > 2: stream is wrongly encoded
> > > > 
> > > > Yes, (1.) is true, forgot to mention it.
> > > Then the ideal fix should go into h264parse I believe :)
> > 
> > This boils down to what you said in comment #39, right?
> 
> Yes. But would be great if you can file a separate bug explaining the reason
> for required changes in codecparser. Patch to fix this would be nice too :)

bug 786797

Created attachment 358431 [details] [review]
libs: decoder: h264: decode SVC base view only

Compared to previous patches:
- Differences between handling MVC and SVC base-only are removed. gst_h264_parse_sps() both for MVC and SVC.
- Changes to gst_vaapi_decoder_h264_parse() only for the priv->base_only == TRUE case. I don't touch FRAME START flags for anything other than prefix NALUs.

Review of attachment 358431 [details] [review]:

::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c
@@ +4716,3 @@
   pi->flags = flags;
+  if (!priv->base_only || pi->nalu.type != GST_H264_NAL_PREFIX_UNIT)
+    gst_vaapi_parser_info_h264_replace (&priv->prev_pi, pi);

For a base-only use case, why shouldn't we treat both prefix_nal and slice_ext in a similar way?
Here both slice_ext and prefix_nal are set for SKIP. But slice_ext still get stored in prev_pi??

(In reply to sreerenj from comment #51)
> Review of attachment 358431 [details] [review] [review]:
> 
> ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c
> @@ +4716,3 @@
>    pi->flags = flags;
> +  if (!priv->base_only || pi->nalu.type != GST_H264_NAL_PREFIX_UNIT)
> +    gst_vaapi_parser_info_h264_replace (&priv->prev_pi, pi);
> 
> For a base-only use case, why shouldn't we treat both prefix_nal and
> slice_ext in a similar way?
> Here both slice_ext and prefix_nal are set for SKIP. But slice_ext still get
> stored in prev_pi??

The reason for this change is because PREFIX NALs get used in parse_slice() which leads to the MVC freeze issue from comment #42. I didn't see a reason to include slice_ext.

(In reply to Orestis Floros from comment #52)
> (In reply to sreerenj from comment #51)
> > Review of attachment 358431 [details] [review] [review] [review]:
> > 
> > ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c
> > @@ +4716,3 @@
> >    pi->flags = flags;
> > +  if (!priv->base_only || pi->nalu.type != GST_H264_NAL_PREFIX_UNIT)
> > +    gst_vaapi_parser_info_h264_replace (&priv->prev_pi, pi);
> > 
> > For a base-only use case, why shouldn't we treat both prefix_nal and
> > slice_ext in a similar way?
> > Here both slice_ext and prefix_nal are set for SKIP. But slice_ext still get
> > stored in prev_pi??
> 
> The reason for this change is because PREFIX NALs get used in parse_slice()
> which leads to the MVC freeze issue from comment #42. I didn't see a reason
> to include slice_ext.

Here the base-only == AnnexA based stream. If so, why we still considering slice_extenion for frame/au boundary detection?

Created attachment 358456 [details] [review]
libs: decoder: h264: remove non-Annex-A nals for base-only

(In reply to sreerenj from comment #53)
> (In reply to Orestis Floros from comment #52)
> > (In reply to sreerenj from comment #51)
> > > Review of attachment 358431 [details] [review] [review] [review] [review]:
> > >
> > > ::: gst-libs/gst/vaapi/gstvaapidecoder_h264.c
> > > @@ +4716,3 @@
> > >    pi->flags = flags;
> > > +  if (!priv->base_only || pi->nalu.type != GST_H264_NAL_PREFIX_UNIT)
> > > +    gst_vaapi_parser_info_h264_replace (&priv->prev_pi, pi);
> > >
> > > For a base-only use case, why shouldn't we treat both prefix_nal and
> > > slice_ext in a similar way?
> > > Here both slice_ext and prefix_nal are set for SKIP. But slice_ext still get
> > > stored in prev_pi??
> >
> > The reason for this change is because PREFIX NALs get used in parse_slice()
> > which leads to the MVC freeze issue from comment #42. I didn't see a reason
> > to include slice_ext.
>
> Here the base-only == AnnexA based stream. If so, why we still considering
> slice_extenion for frame/au boundary detection?

I generally tried to not modify the process as much in the spirit of bug 732265. In the end, I believe that actually dropping NALs 14, 15, 20 is more elegant, less hackish and I think a bit faster too.

This patch here shows the difference in complexity.

Created attachment 358457 [details] [review]
libs: decoder: h264: decode SVC base view only

Alternatively, squashed with the patch before

Review of attachment 358457 [details] [review]:

Nice, This solution is clean.
Can you add few lines of explanation in the commit message? 
Please obsolete the other patches except for the one to gstvaapidecode.c.

Created attachment 358495 [details] [review]
libs: decoder: h264: decode SVC base layer only

(In reply to sreerenj from comment #56)
> Review of attachment 358457 [details] [review] [review]:
> Nice, This solution is clean.
> Can you add few lines of explanation in the commit message?

Tell me if you want to move some of these as comments inside the code or if I am being too verbose.

(In reply to Orestis Floros from comment #57)
> Created attachment 358495 [details] [review] [review]
> libs: decoder: h264: decode SVC base layer only
> 
> (In reply to sreerenj from comment #56)
> > Review of attachment 358457 [details] [review] [review] [review]:
> > Nice, This solution is clean.
> > Can you add few lines of explanation in the commit message?
> 
> Tell me if you want to move some of these as comments inside the code or if
> I am being too verbose.

Always prefer to have more comments inside the code, and a good enough commit message too. Personally I don't mind even if it is too long :)

Created attachment 358538 [details] [review]
libs: decoder: h264: decode SVC base layer only

(In reply to sreerenj from comment #58)
> (In reply to Orestis Floros from comment #57)
> > Created attachment 358495 [details] [review] [review] [review]
> > libs: decoder: h264: decode SVC base layer only
> >
> > (In reply to sreerenj from comment #56)
> > > Review of attachment 358457 [details] [review] [review] [review] [review]:
> > > Nice, This solution is clean.
> > > Can you add few lines of explanation in the commit message?
> >
> > Tell me if you want to move some of these as comments inside the code or if
> > I am being too verbose.
>
> Always prefer to have more comments inside the code, and a good enough
> commit message too. Personally I don't mind even if it is too long :)

Ok, I added some.

I actually found out that most of the process in gst_vaapi_decoder_h264_parse() can be skipped. This should be a bit more efficient and also only adds a block of code in gst_vaapi_decoder_h264_parse() instead of multiple changes.

I think it's preferable.

(In reply to Orestis Floros from comment #59)
> Created attachment 358538 [details] [review] [review]
> libs: decoder: h264: decode SVC base layer only

Does this patch obsolete attachment 358495 [details] [review]?

If so, please mark it accordingly.

(In reply to Víctor Manuel Jáquez Leal from comment #60)
> (In reply to Orestis Floros from comment #59)
> > Created attachment 358538 [details] [review] [review] [review]
> > libs: decoder: h264: decode SVC base layer only
> 
> Does this patch obsolete attachment 358495 [details] [review] [review]?
> 
> If so, please mark it accordingly.

attachment 358495 [details] [review] was something that we agreed on with sreerenj that's why I haven't marked it as obsolete yet.

Review of attachment 358538 [details] [review]:

lgtm. I will push these patches.

(In reply to sreerenj from comment #62)
> Review of attachment 358538 [details] [review] [review]:
> 
> lgtm. I will push these patches.

There are going to be some problems without the patches from bug 732267 but it shouldn't break MVC streams and it will work fine for most SVC streams.

(In reply to Orestis Floros from comment #63)
> (In reply to sreerenj from comment #62)
> > Review of attachment 358538 [details] [review] [review] [review]:
> > 
> > lgtm. I will push these patches.
> 
> There are going to be some problems without the patches from bug 732267 but
> it shouldn't break MVC streams and it will work fine for most SVC streams.

Yup. We can push the gstremaer-vaapi patches now and then focuss on #732267.

Quick questions: Do we really need to parse the subset SPS at all? :)

Review of attachment 358538 [details] [review]:

Have to figure out the subset_sps parse issue.

The problem here is that you set the nal units for SKIP, but returned from the parse function without parsing and eventually with out setting the flags (FRAME_START, FRAME_END etc). As a result, all of these non-annex-a nal units get accumulated as pre_units of the GstVaapiParserFrame. Ideally, pre_units shouldn't have the slice nals. But here we store the SLCE_EXT slice nal also in preunits (even though it is set for skip).

Then later when ParserFrame gets unreferenced, it tries to invoke the sps_clear() and pps_clear() assuming that the data has been parsed and stored already.

That could be the reason you get the segfault in subset_sps handling. You may have the similar issue with pps_clear too since  parse_pps just returns success for GST_H264_PARSER_BROKEN_LINK.

By the way, isn't the code in gstvaapidecoder_h264.c:4589

> if (priv->is_avcC)
>   result = gst_h264_parser_identify_nalu_avc (priv->parser,
>       buf, 0, buf_size, priv->nal_length_size, &pi->nalu);
> else
>   result = gst_h264_parser_identify_nalu_unchecked (priv->parser,
>       buf, 0, buf_size, &pi->nalu);
> status = get_status (result);

dangerous in the same way? If h264parser fails to parse them and they have garbage data (eg as with the PPS case you mention) it can lead to a segfault in _finalize() which is called after goto exit IIUC.

This is a bit of uncharted territory for me. A workaround for this without parsing the ssps could be to mark the nalu as not valid and check in _finalize():

> diff --git a/gst-libs/gst/vaapi/gstvaapidecoder_h264.c b/gst-libs/gst/vaapi/gstvaapidecoder_h264.c
> index 8b8a08ee..f779ac71 100644
> --- a/gst-libs/gst/vaapi/gstvaapidecoder_h264.c
> +++ b/gst-libs/gst/vaapi/gstvaapidecoder_h264.c
> @@ -100,6 +100,9 @@ struct _GstVaapiParserInfoH264
>  static void
>  gst_vaapi_parser_info_h264_finalize (GstVaapiParserInfoH264 * pi)
>  {
> +  if (!pi->nalu.valid)
> +    return;
> +
>    switch (pi->nalu.type) {
>      case GST_H264_NAL_SPS:
>      case GST_H264_NAL_SUBSET_SPS:
> @@ -1811,8 +1814,10 @@ parse_pps (GstVaapiDecoderH264 * decoder, GstVaapiDecoderUnit * unit)
>    result = gst_h264_parser_parse_pps (priv->parser, &pi->nalu, pps);
>
>    /* PPS's sps id might be an ignored subset sps in SVC streams */
> -  if (priv->base_only && result == GST_H264_PARSER_BROKEN_LINK)
> +  if (priv->base_only && result == GST_H264_PARSER_BROKEN_LINK){
> +    pi->nalu.valid = FALSE;
>      return GST_VAAPI_DECODER_STATUS_SUCCESS;
> +  }
>
>    priv->parser_state &= GST_H264_VIDEO_STATE_GOT_SPS;
>
> @@ -4593,6 +4598,7 @@ gst_vaapi_decoder_h264_parse (GstVaapiDecoder * base_decoder,
>            || pi->nalu.type == GST_H264_NAL_SUBSET_SPS
>            || pi->nalu.type == GST_H264_NAL_SLICE_EXT)) {
>      GST_VAAPI_DECODER_UNIT_FLAG_SET (unit, GST_VAAPI_DECODER_UNIT_FLAG_SKIP);
> +    pi->nalu.valid = FALSE;
>      return GST_VAAPI_DECODER_STATUS_SUCCESS;
>    }
>    switch (pi->nalu.type) {

(In reply to Orestis Floros from comment #67)
> By the way, isn't the code in gstvaapidecoder_h264.c:4589
> 
> > if (priv->is_avcC)
> >   result = gst_h264_parser_identify_nalu_avc (priv->parser,
> >       buf, 0, buf_size, priv->nal_length_size, &pi->nalu);
> > else
> >   result = gst_h264_parser_identify_nalu_unchecked (priv->parser,
> >       buf, 0, buf_size, &pi->nalu);
> > status = get_status (result);
> 
> dangerous in the same way? If h264parser fails to parse them and they have
> garbage data (eg as with the PPS case you mention) it can lead to a segfault
> in _finalize() which is called after goto exit IIUC.
> 

There is nothing to do with what h264pasr has been parsed or not.
But yes, there is a possibility to crash in this scenario since we are not zero initializing the parser_info. Easy fix for this is to use gst_vaapi_mini_object_new0() instead of gst_vaapi_mini_object_new() for GstVaapiParserInfoH264 creation.

Created attachment 358642 [details] [review]
libs: decoder: remove custom ssps check for base_only

This is what we are discussing with sreerenj. Removing this check causes segfault in some SVC streams for me.

I'll squash this later.

Created attachment 358643 [details] [review]
libs: decoder: h264: check nalu validity in parser info finalize

And this is a workaround that fixes the segfaults.

I haven't tested, but lgtm. Please squash them and obsolete the older ones.

BTW gst_h264_parser_identify_nalu_unchecked() already doing the memset for every nalu. So we shouldn't have the problem mentioned in comment 67 and comment 68.

Created attachment 358645 [details] [review]
libs: decoder: h264: check nalu validity in parser info finalize

Created attachment 358646 [details] [review]
libs: decoder: h264: decode SVC base layer only

Review of attachment 357927 [details] [review]:

Pushed, commit id: 1ae42facc170c48d889fab2e6c9a8430f2c79899

Review of attachment 358645 [details] [review]:

Pushed, commit id: a016aa181b8239e11ab1795c2630c407a7d64811

Review of attachment 358646 [details] [review]:

Pushed, commit id: 09557528983edc36414e8d62b836fc012027db7a