Hrm, sorry for the long wait. So what's up with the

 /*Fixme: Not checking for 'Data Partitioning Mode' of Sequence Scalable Extension */

? Isn't that API-relevant?

If I'm reading the spec right we'd need to pass an additional parameter with the sequence_scalable_extension (even if we don't have a struct for that yet or actually parse it).

(In reply to comment #1)
> Hrm, sorry for the long wait. So what's up with the
> 
>  /*Fixme: Not checking for 'Data Partitioning Mode' of Sequence Scalable
> Extension */
> 
> ? Isn't that API-relevant?
> 
> If I'm reading the spec right we'd need to pass an additional parameter with
> the sequence_scalable_extension (even if we don't have a struct for that yet or
> actually parse it).

I have a patch for Sequence Scalable Extension parsing. Do I need to file a separate bug for that or is it enough to add the patch here.?

How about changing the API 
    gst_mpeg_video_parse_slice(Slice, 
                               Seq, SeqExt, SeqScalExt, 
                               data, size, offset); 

to

   gst_mpeg_video_parse_slice(Slice,
                              vertical_size, SeqScalExt,
                              data, size, offset);


Because now the mpegparser has gst_mpeg_video_finalise_mpeg2_sequence_header() api which is calculating the vertical_size also.

Adding it here is fine, thanks!

Created attachment 234396 [details] [review]
 mpegvideoparser: Add Sequence Scalable Extension parsing API

(In reply to comment #3)
> Adding it here is fine, thanks!

Done..
What do you think about the gst_mpeg_video_parse_slice() API ?

(In reply to comment #5)
> (In reply to comment #3)
> > Adding it here is fine, thanks!
> 
> Done..
> What do you think about the gst_mpeg_video_parse_slice() API ?

I prefer the following data structure:

struct _GstMpegVideoSliceHdr {
    guint16 slice_horizontal_position;
    guint16 slice_vertical_position;
    guint8 quantiser_scale_code;
    guint8 intra_slice;

    /* Size of the slice() header in bits */
    guint header_size;
};

i.e. the macroblock_address needs to be correctly calculated, otherwise it's useless for gst-vaapi purposes. Please have a look at the current gstvaapidecoder_mpeg2.c:parse_slice() for a more correct implementation.

My copy of the MPEG-2 standard does not mention the picture_id, but if you see a need for it, just add it then. :)

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > Adding it here is fine, thanks!
> > 
> > Done..
> > What do you think about the gst_mpeg_video_parse_slice() API ?
> 
> I prefer the following data structure:
> 
> struct _GstMpegVideoSliceHdr {
>     guint16 slice_horizontal_position;
>     guint16 slice_vertical_position;
>     guint8 quantiser_scale_code;
>     guint8 intra_slice;
> 
>     /* Size of the slice() header in bits */
>     guint header_size;
> };
> 
> i.e. the macroblock_address needs to be correctly calculated, otherwise it's
> useless for gst-vaapi purposes. Please have a look at the current
> gstvaapidecoder_mpeg2.c:parse_slice() for a more correct implementation.
> 
> My copy of the MPEG-2 standard does not mention the picture_id, but if you see
> a need for it, just add it then. :)

I will check that..thanks :)
So what do you think about the API parameters change I mentioned in comment2 ?

or if you guys prefer to do slice_parsing in deocoders itself(with out having a separate API) , then we can do it like that. I have no objection :)

(In reply to comment #7)
> So what do you think about the API parameters change I mentioned in comment2 ?

A seqhdr is probably better than vertical_size since, as you mentioned, vertical_size is already available there and this makes the parameters more uniform with other parsers.

> or if you guys prefer to do slice_parsing in deocoders itself(with out having a
> separate API) , then we can do it like that. I have no objection :)

Yes, I'd like to move GstMpegVideoSliceHdr from gst-vaapi to gst-plugins-bad codecparsers. So, the structure I described is the minimal one I need. :)

(In reply to comment #8)
> (In reply to comment #7)
> > So what do you think about the API parameters change I mentioned in comment2 ?
> 
> A seqhdr is probably better than vertical_size since, as you mentioned,
> vertical_size is already available there and this makes the parameters more
> uniform with other parsers.
> 
> > or if you guys prefer to do slice_parsing in deocoders itself(with out having a
> > separate API) , then we can do it like that. I have no objection :)
> 
> Yes, I'd like to move GstMpegVideoSliceHdr from gst-vaapi to gst-plugins-bad
> codecparsers. So, the structure I described is the minimal one I need. :)

Yup, you are right. Doesn't need to pass the SeqExt to slice_parse() since seq_hdr has the vertical size..
Thanks..

Created attachment 234723 [details] [review]
mpegvideoparse: Add Slice Header parsing API.

Review of attachment 234723 [details] [review]:

Hi,
- it seems that gst_mpeg_video_parse_slice_header() args are not vertically aligned with the other functions;
- slice_vertical_position_extension is useless since it depends on seqhdr values, i.e. it may not convey meaningful value from within the same data structure;
- slice_picture_id has no comment on what it means;
- I really want header_size to be the last field with the following comment: /* Size of the slice() header in bits */
- Missing leading space in /* Calculated values */
- I prefer mb_x, mb_y to mb_row, mb_column even if the latter comes from the specs terminology.

Thanks,
Gwenole.

(In reply to comment #11)
> Review of attachment 234723 [details] [review]:
> 
> Hi,
> - it seems that gst_mpeg_video_parse_slice_header() args are not vertically
> aligned with the other functions;

we need the start_code to get the type which is needed to calcuate mb_x/mb_row..
otherwise we have to pass the slice_no as the next argument..(we already have 6 arguments :))

> - slice_vertical_position_extension is useless since it depends on seqhdr
> values, i.e. it may not convey meaningful value from within the same data
> structure;

have no objection to avoid that field :)

> - slice_picture_id has no comment on what it means;

just kept that field since it might need for some once else even though it is not using for decoding.But i don't know whether someone is using that or not...Is it an overhead?
As per the spec,

"slice_picture_id is intended to aid recovery on severe bursts of
errors for certain types of applications. For example, the application may increment slice_picture_id with each transmitted picture, so that in case of severe burst error, when several slices are lost, the decoder can know if the slice following the burst error belongs to the current picture or to another picture, which may be the case if at least a picture
header has been lost."


> - I really want header_size to be the last field with the following comment: /*
> Size of the slice() header in bits */

Sorry i didn't get it.Does the calculation is wrong? remember the data is starting from the start_code.

> - Missing leading space in /* Calculated values */
> - I prefer mb_x, mb_y to mb_row, mb_column even if the latter comes from the
> specs terminology.

no objection .  

> Thanks,
> Gwenole.

Created attachment 234829 [details] [review]
codecparsers: mpegvideo: Add Slice Header parsing API.

Based on comment11 and comment12,
--fixed the white space warning
--add comment for slice_picture_id
--removed the slice_vertical_position_extension field from the header
--but retained the mb_row/mb_colum because otherwise it will be a confusion for someone else who is checking with the spec.

Review of attachment 234829 [details] [review]:

Hi, on second thought, the slice API is not fully consistent with the rest (seq_hdr, pic_hdr, et al.). In particular, the offset for slice() is relative to the start-code prefix, whereas it is relative to the actual data (after the start code + type) for others. I suggest we globally change the MPEG-2 video parser API to take a const GstMpegVideoPacket * instead of multiple arguments like data, size, offset. That way, the slice API would also know about the type to calculate slice_vertical_position without parsing it again.

Besides, this would also simplify a bit client applications since they usually know about the GstMpegVideoPacket to use and would just need to pass it along to the parser, instead of propagating all its fields as multiple arguments.

That's on Tim's call now. :)

BTW, you don't need to check for height > 2800 again if slice_vertical_position is already initialized to zero, before hand. i.e. shift + add is probably cheaper than a mispredicted branch in the common case. Not that this is really important, just something that came to my mind. :)

Review of attachment 234829 [details] [review]:

If we want to preserve ABI, the better alternative is to add gst_mpeg_video_packet_parse_*() functions, which now hold the default implementations. Others are marked are deprecated and they call into the new gst_mpeg_video_packet_*() functions. I will probably provide a patch later today.

(In reply to comment #14)
> Review of attachment 234829 [details] [review]:
> 
> Hi, on second thought, the slice API is not fully consistent with the rest
> (seq_hdr, pic_hdr, et al.). In particular, the offset for slice() is relative
> to the start-code prefix, whereas it is relative to the actual data (after the
> start code + type) for others. I suggest we globally change the MPEG-2 video
> parser API to take a const GstMpegVideoPacket * instead of multiple arguments
> like data, size, offset. That way, the slice API would also know about the type
> to calculate slice_vertical_position without parsing it again.
> 
> Besides, this would also simplify a bit client applications since they usually
> know about the GstMpegVideoPacket to use and would just need to pass it along
> to the parser, instead of propagating all its fields as multiple arguments.
> 

Seems fine...We might be able to do the same with RBDU in vc1,,,

> That's on Tim's call now. :)
> 
> BTW, you don't need to check for height > 2800 again if slice_vertical_position
> is already initialized to zero, before hand. i.e. shift + add is probably
> cheaper than a mispredicted branch in the common case. Not that this is really
> important, just something that came to my mind. :)

Hm,,second if() check is already an extra branching since it is possible to do the mb_row calculation in first if() check itself..
What you said might be more optimized...
Thanks for the catch..
Do i need to update it like that?? Or will you do it together with your proposed gst_mpeg_video_packet_parse_*() patch?Just asked because you mentioned that you might come up with the patch later today itself :)

Created attachment 234906 [details] [review]
codecparsers:mpegvideo : optimise the slice:mb_row calculation.

(In reply to comment #16)
> (In reply to comment #14)
> > Besides, this would also simplify a bit client applications since they usually
> > know about the GstMpegVideoPacket to use and would just need to pass it along
> > to the parser, instead of propagating all its fields as multiple arguments.
> > 
> 
> Seems fine...We might be able to do the same with RBDU in vc1,,,

Maybe, but I have other plans for the VC-1 parser. :)

> > That's on Tim's call now. :)
> > 
> > BTW, you don't need to check for height > 2800 again if slice_vertical_position
> > is already initialized to zero, before hand. i.e. shift + add is probably
> > cheaper than a mispredicted branch in the common case. Not that this is really
> > important, just something that came to my mind. :)
> 
> Hm,,second if() check is already an extra branching since it is possible to do
> the mb_row calculation in first if() check itself..
> What you said might be more optimized...
> Thanks for the catch..
> Do i need to update it like that?? Or will you do it together with your
> proposed gst_mpeg_video_packet_parse_*() patch?Just asked because you mentioned
> that you might come up with the patch later today itself :)

I think we should first settle with the new GstMpegVideoPacket based API first, then you can update your patches after that.

Any  chance to push these patches??
I will add necessary patches to https://bugzilla.gnome.org/show_bug.cgi?id=692933 after that...

Comment on attachment 234906 [details] [review]
codecparsers:mpegvideo : optimise the slice:mb_row calculation.

I don't really have an opinion on this patch, it seems like a micro-optimisation that surely is not going to really make a difference? If in doubt, I tend to prefer clear code (but again, here it hardly matters). Anyway, I'll leave it to Gwenole to decide.

If you do push it, please fix up the commit message (weird space placement in subject line, and coparison->comparison) and the comments in the code (use '*' in each comment line, and also coparison->comparison).

The other two look fine to me if Gwenole is happy with them too.

I think bug #692933 should be done first though?

(In reply to comment #15)
> Review of attachment 234829 [details] [review]:
> 
> If we want to preserve ABI, the better alternative is to add
> gst_mpeg_video_packet_parse_*() functions, which now hold the default
> implementations. Others are marked are deprecated and they call into the new
> gst_mpeg_video_packet_*() functions. I will probably provide a patch later
> today.

FWIW, I don't mind breaking ABI here, as long as there's a good reason to change it. If there is, the sooner the better IMHO (and please also update the video parser then).

Applying the patch in https://bugzilla.gnome.org/show_bug.cgi?id=692933 to the current git master of plugin-bad is failing. Something has been changed in git master i think....
I will wait for Gwenole to push those patches then...

I would like to use the "GstMpegVideoMeta" from plugins-bad for gstreamer-vaapi also. For that we need to get this API in. Otherwise the gstreamer-vaapi needs to parse the data again to get the slice_hdr details.

I'm waiting for feedback from Gwenole. My comment #20 and comment #21 still apply :)

Milestoning as 1.1.x though, to re-visit before 1.2 hopefully.

Hi Gwenole, Could you please have a look into this?

(In reply to comment #25)
> Hi Gwenole, Could you please have a look into this?

Ah, sorry, I thought you only needed me to review the dependency.

OK, so for the slice_hdr parser, you can now use GstMpegVideoPacket interface. Don't bother with adding the gst_mpeg_video_parse_slice_header() into the deprecated namespace, the args would not match other functions. i.e. only add a gst_mpeg_video_packet_parse_slice_header() function, the slice_vertical_position would come from the packet->type. Thanks.

Otherwise, that looks fine, assuming it still works with gstreamer-vaapi. :)

Created attachment 248452 [details] [review]
codecparsers: mpegvideo: Add Sequence scalable extension parsing API

Created attachment 248453 [details] [review]
codecparsers: mpegvideo: Add Slice Header parsing API.

Updated the patches on top of Gwenole's patches in https://bugzilla.gnome.org/show_bug.cgi?id=692933

Review of attachment 248452 [details] [review]:

::: gst-libs/gst/codecparsers/gstmpegvideoparser.h
@@ +124,1 @@
 } GstMpegVideoLevel;

Unrelated changes, maybe due to gst-indent?

@@ +316,3 @@
+  guint8 vertical_subsampling_factor_n;
+
+  /*if temporal scalability*/

/* if ... */ -- missing spaces.

Review of attachment 248453 [details] [review]:

::: gst-libs/gst/codecparsers/gstmpegvideoparser.c
@@ +960,3 @@
+  guint height;
+  guint mb_inc;
+  guint8 bits = 0, extra_bits = 0;

Junk bytes don't need to be initialized.

@@ +961,3 @@
+  guint mb_inc;
+  guint8 bits = 0, extra_bits = 0;
+  guint8 vertical_position = 0, vertical_position_extension = 0;

Only vertical_position_extension needs to be zero-initialized, not vertical_position.

@@ +972,3 @@
+
+  SKIP (&br, 24);
+  vertical_position = gst_bit_reader_get_bits_uint8_unchecked (&br, 8);

I don't think this will work since the offset is relative to the actual slice header start, without the start_code_prefix.

Created attachment 248456 [details] [review]
codecparsers: mpeg2: add sequence scalable extension parsing API

Actually, I was also working on updating the patches. :)

Created attachment 248457 [details] [review]
codecparsers: mpeg2: add slice header parsing API

(In reply to comment #30)
> Review of attachment 248452 [details] [review]:
> 
> ::: gst-libs/gst/codecparsers/gstmpegvideoparser.h
> @@ +124,1 @@
>  } GstMpegVideoLevel;
> 
> Unrelated changes, maybe due to gst-indent?
> 
> @@ +316,3 @@
> +  guint8 vertical_subsampling_factor_n;
> +
> +  /*if temporal scalability*/
> 
> /* if ... */ -- missing spaces.

No gst-indent for headers. But yes, there was missing space before GStMpegVideoLevel which i have corrected on the way.

Thanks for the review :)

Created attachment 248461 [details] [review]
codecparsers: mpeg2: add slice header parsing API

Fix off by one error in mb_row calculation. The slice_vertical_position shall start from zero.

Comment on attachment 248461 [details] [review]
codecparsers: mpeg2: add slice header parsing API

Looks fine to me, but please add 'Since: 1.2' markers to the gtk-doc chunks for the new function and the new structure.

Comment on attachment 248456 [details] [review]
codecparsers: mpeg2: add sequence scalable extension parsing API

Looks fine. Please also add a 'Since 1.2' to the structure doc chunk.

Argh, I did not commit the right patch. Fixing now, missing tests/ update. Sorry.

All the patches are landed in upstream. Please close this bug.

commit 9a2ed785321568dbb568ed485d4310ee12b74008
Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
Date:   Wed Jan 30 16:56:15 2013 +0200

    codecparsers: mpeg2: add slice header parsing API.
    
    Add API to parse the Slice header. This also calculates the macroblock
    position as specified in 6.3.16.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=664274
    
    Signed-off-by: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
    Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>

commit 2adfbe36f16cf9d69d91cdf5ee4ecd3a4bbfc858
Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
Date:   Fri Jan 25 15:48:58 2013 +0200

    codecparsers: mpeg2: add sequence scalable extension parsing API.
    
    Add API to parse the Sequence Scalable Extension header.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=664274
    
    Signed-off-by: Sreerenj Balachandran <sreerenj.balachandran@intel.com>
    Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>