Mpeg video support is now upstream. h264 needs a tad more work

commit 67ac84c7b570750200b60cc5c1032c496298e564
Author: Edward Hervey <edward.hervey@collabora.co.uk>
Date:   Thu Sep 20 18:03:59 2012 +0200

    codecparsers: Add Mpeg Video GstMeta
    
    This can be used by parsers to provide pre-parsed information to
    downstream elements that would require it (so they can avoid having
    to parse the bitstream again).

<sree_> bilboded: need more changes to make [GstMpegVideoMeta] usable for gstreamer-vaapi (i think)
<__tim> sree_, well, it would be good to know more details about that, ideally soon (before 1.2.0)
* bilboed nods
<__tim> an we should hold off on the other metas until then IMHO
<slomo> agreed
<sree_> __tim: okay i will try to look into that soon. When are you expecting the 1.2.0 release?
<__tim> sree_, not for a while, but do you think you could you take a look at it this week maybe? Just so we have a general idea what the issues are?
<sree_> __tim: sure. i will try

We need to get all the GstMpegVideoSliceHdr of each frame for gstreamer-vaapi.
I am thinking of adding a GArray to hold all GstMpegVideoSliceHdrs in GstMpegVideoMeta.
The size of meta->slice_hdr_array == meta->num_slices;
suggestions?

The VAAPI have to provide slice details separately for each slice. Which means we need to get all the GstMpegVideoSliceHdr of each frame for gstreamer-vaapi.
Another change is needed for slice_offset field in GstMpegMeta. Currently slice_offset is giving the offset of the first slice of a frame. Either we need to move the slice_offset field to GstMpegVideoSliceHdr or some other way to communitate all slice offsets through GstMpegMeta.

I am thinking of adding a GArray to GstMpegVideoMeta which will hold all GstMpegVideoSliceHdrs of a parsed frame.
The size of meta->slice_hdr_array == meta->num_slices;

Suggestions?

We would also need some sort of negotiation with downstream decoder about what it needs to be parsed. e.g. VDPAU is a frame-level API, and thus is fine with only knowing about the number of slices, but VA-API is a slice-level API, and needs info.

Okay,, So if the downstream supports only frame_level api then the parser doesn't need to parse all the slices since it is bit expensive. And if the downstream supports slice_level api, then the parser should parse every slices.

METHOD1:
--------------
How about splitting GstMpegVideoMeta in to two Metas:

--The GstMpegVideoMeta for all the generic headers like seq_hdr, pic_hdr etc

--The GstMpegVideoSliceMeta to keep an array of GstMpegVideoSliceHdr and slice_offsets

So as per the request from downstream elements, parser can add the necessary metas:

METHOD2:
--------------
Otherwise we can use an API specific parameter need_slice_parsing.
So the downstream elements which need to get the slice information should add "need_slice_parsing=TRUE" to the GstStructure field @parm in gst_query_add_allocation_meta().

And we can keep an array of GstMpegVideoSliceS with in GstMpegVideoMeta to store both GstMpegVideoSliceHdr and slice_offset.

The 2nd method is cleaner imho. There's no need adding yet-another-meta for just one extra field.

Makes sense. Waiting for patches :)

(In reply to comment #7)
> The 2nd method is cleaner imho. There's no need adding yet-another-meta for
> just one extra field.
> 

Actually it is not just a field. We need an array to keep the GstMpegVideoSliceHdrs of all slices in the frame. Also needs to keep an offset  field for each slice.

> Makes sense. Waiting for patches :)

Anyway I will proceed with method 2 :)..Thanks.

Hi, yes, a single meta is enough. Thanks.

BTW, I am still not convinced this will bring a lot. My current measurements for h264 show that parsing takes around 0.2 ms, even on a slower Sandybridge underclocked at 800 MHz. i.e. there are probably other hotspots to optimize. :)

What I mean is that, it's a good thing, but probably not as good as something else. Anyway, that wouldn't require much time either.

I have not measured for MPEG-2. I have a pathological case where someone decided to encode a different slice per macroblock. So, please also be careful about what you intend to allocate.

Hi Gwenole,

Does it enough for your use-cases?
 
struct _GstMpegVideoMetaSlice {
  GstMpegVideoSliceHdr *slice_hdr;
  gsize slice_offset;  /* the offset of the slice with in the buffer */
};

struct _GstMpegVideoMeta {
....
....
....
  GArray *slice_array;  /* an array of GstMpegVideoMetaSlice */
};

(In reply to comment #10)
> Hi Gwenole,
> 
> Does it enough for your use-cases?
> 
> struct _GstMpegVideoMetaSlice {
>   GstMpegVideoSliceHdr *slice_hdr;
>   gsize slice_offset;  /* the offset of the slice with in the buffer */
> };
> 
> struct _GstMpegVideoMeta {
> ....
> ....
> ....
>   GArray *slice_array;  /* an array of GstMpegVideoMetaSlice */
> };

Yes, I think so + slice_size (including the macroblock layer). Maybe GstMpegVideoMetaSliceInfo would be more informative + slice_info array member? Thanks.

I was thinking to calculate the slice size with in the decoder. :) I will add that anyways.

Thanks..

> struct _GstMpegVideoMetaSlice {
>   GstMpegVideoSliceHdr *slice_hdr;
>   gsize slice_offset;  /* the offset of the slice with in the buffer */
> };

Why not embed the SliceHdr structure into the MetaSlice structure here? Does it point towards an allocation existing somewhere else?

We are doing g_slice_dup() for each headers in gstmpegvideometa implementation. 
Is that an answer to your question?(In reply to comment #13)
> > struct _GstMpegVideoMetaSlice {
> >   GstMpegVideoSliceHdr *slice_hdr;
> >   gsize slice_offset;  /* the offset of the slice with in the buffer */
> > };
> 
> Why not embed the SliceHdr structure into the MetaSlice structure here? Does it
> point towards an allocation existing somewhere else?

We are doing g_slice_dup() for each headers with in gstmpegvideometa implementation. 
Is that an answer to your question?

> Is that an answer to your question?

Not really? :)

(In reply to comment #13)
> > struct _GstMpegVideoMetaSlice {
> >   GstMpegVideoSliceHdr *slice_hdr;
> >   gsize slice_offset;  /* the offset of the slice with in the buffer */
> > };
> 
> Why not embed the SliceHdr structure into the MetaSlice structure here? Does it
> point towards an allocation existing somewhere else?

Yes, you are right, that's even less allocations.

BTW, we should also define more precisely the offset to e.g. include the start code prefix (or not). Thanks.

The mpegvideoparse element is allocating memory for all other headers like SeqHdr, PicHdr etc. So I thought it would be good to keep the same approach for SliceHdrs also. But it could be bit more simple to keep the SliceHdrs inside GstMpegVideoMetaSliceInfo so that the parser doesn't need to allocate separately for SliceHdr and GstMpegVideoMetaSliceInfo. 
Thanks.

Created attachment 249503 [details] [review]
mpegvideometa: Add slice information to the GstMpegVideoMeta

Created attachment 249504 [details] [review]
mpegvideoparse: Store slice information in meta if downstream needs it

Review of attachment 249503 [details] [review]:

Hi, I would prefer the GArray be an array of GstMpegVideoSliceInfo elements, not pointers to GstMpegVideoSliceInfo structs.

::: gst-libs/gst/codecparsers/gstmpegvideometa.c
@@ +124,3 @@
+    slice_info_src =
+        g_array_index (slice_info_array, GstMpegVideoMetaSliceInfo *, i);
+    slice_info_dst = g_slice_dup (GstMpegVideoMetaSliceInfo, slice_info_src);

A GArray of pointers is called a GPtrArray. :)

@@ +128,3 @@
+  }
+}
+

I would rename gst_mpeg_video_meta_add_slice_info() to gst_mpeg_video_meta_set_slice_info() and simply g_array_ref() the slice_info there.

Review of attachment 249504 [details] [review]:

I think it's possible to use GstMpegVideoSliceInfo elements as is, instead of allocating and storing pointers to thoat struct. At least, this is what I would like to have, so that to avoid too many dynamic allocations. :)

::: gst/videoparsers/gstmpegvideoparse.c
@@ +636,3 @@
+    slice_info->slice_size = (off - 4) - mpvparse->slice_offset;
+    g_array_append_val (mpvparse->slice_info_array, slice_info);
+

Can't we just g_array_set_size() +
{
  GstMpegVideoMetaSliceInfo * const slice_info =
    &g_array_index(mpvparse->slice_info_array, GstMpegVideoMetaSliceInfo, i);
  and directly parse_slice_header() in there?
}

Argh, GArray does not provide a means to append an empty element that we are going to modify. i.e. set_size() would grow the allocation, not the number of elements, thus still requiring to copy the data.

Yeah, seems so, but at least we can avoid the dynamic allocation for each SliceInfo.

It might also be possible to do something like expose a NULL-terminated GstMpegVideoMetaSliceInfo ** in the structure instead of a GArray (I *think* g-i handles NULL-terminated pointer arrays fine, just not if it needs to get the size of the array from a different structure member). Then you could basically do g_malloc (sizeof (gpointer) * (num_slices + 1) + align + (num_slices * sizeof (SliceInfo)) or so, that you just use one malloc altogether, and the pointers point into the same allocation block or so. (I hope I'm not terribly confused now, in which case please don't laugh).

Using SliceInfo instead of (SliceInfo *) will cause an extra memcpy for each g_array_append() . right?

So which one is better in this context

--more dynamic allocation

  --GPtrArray to hold the pointers to GstMpegVideoSliceMetaInfo with in mpvparse
  --Both Mpvparse and MpegMeta are only doing ref()/unref() for the array


-- more memcpy
 
 --GArray to hold the GstMpegVideoSliceMetaInfo itself
 --g_array_append() will cause a memcpy each time we append the SliceInfo
 --MpegMeta will only do a ref()/unref() for the array.


@tim: Do you meant to allocate the memory only once for a single frame? That is not possible since we don't know about the number of slices in a frame in advance.

You can use/fill a GArray efficiently by doing the following:

* Create it with the known number of entries
* iterate and fill directly (MySomething *something = &g_array_index(array, MySomething, i);)
* Once you've filled it, set the array->len to the number of entries

voila. No memcpy

Example usage in mpegts lib: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/mpegts/gstmpegtsdescriptor.c#n519

You might want to check with GLib people that they agree that that's ok to do (keep in mind that not all of the GArray structure is public etc.).

All the fields used are public. And the pre-allocation methods are public also.

(In reply to comment #25)
> You can use/fill a GArray efficiently by doing the following:
> 
> * Create it with the known number of entries

Sorry, in this case what do you mean by the "known number of entries"  : Some random high values (some heuristics based on width and height??) ? or do you meant to pre-calculate the number of slices in each frame before creating/start_to_fill the array?

Am i missing something?

@Gwenole:

I know that you are using the heuristic :num_slices = (height + 15) / 16 for gstreamer-vaapi.:)

Is it safe to utilize the same here?

Ok, so:

1) modifying array->len is not kosher, according to glib developers, but

2) g_array_set_size() *does* set array->len as well, so does what we want.

I think it's ok for starters to just do it the dumb way, i.e. parse into SliceInfo structure on the stack, then call g_array_append_val() causing a memcpy of the struct. We can then improve it later. It's just an implementation detail, no need to block on it.

Created attachment 249660 [details] [review]
mpegvideometa: Add slice information to the GstMpegVideoMeta

Created attachment 249661 [details] [review]
mpegvideoparse: Store slice information in meta if downstream needs it

Now the patches are using SliceInfo directly.

(In reply to comment #30)
> @Gwenole:
> 
> I know that you are using the heuristic :num_slices = (height + 15) / 16 for
> gstreamer-vaapi.:)
> 
> Is it safe to utilize the same here?

It's a good enough heuristic to begin with. Of course, the array could keep growing afterwards, for degenerated cases.

(In reply to comment #33)
> Created an attachment (id=249661) [details] [review]
> mpegvideoparse: Store slice information in meta if downstream needs it

Could you please create a separate patch for gst_mpegv_parse_process_sc() to support GstMpegVideoPacket? I did not do that in the original patch for the migration because there were/could be some interesting corner cases when you hit the gotos around. :)

(In reply to comment #32)
> Created an attachment (id=249660) [details] [review]
> mpegvideometa: Add slice information to the GstMpegVideoMeta

LGTM.

(In reply to comment #36)
> (In reply to comment #33)
> > Created an attachment (id=249661) [details] [review] [details] [review]
> > mpegvideoparse: Store slice information in meta if downstream needs it
> 
> Could you please create a separate patch for gst_mpegv_parse_process_sc() to
> support GstMpegVideoPacket? I did not do that in the original patch for the
> migration because there were/could be some interesting corner cases when you
> hit the gotos around. :)

You might also want to check current git master, just saying :)

Created attachment 249859 [details] [review]
mpegvideoparse: Store slice information in meta if downstream needs it

I have rebased it on top of current git master.Now the mpegvideoparse-patch is using the semantics of Edward's patch 1db3d40a4bdaa3f77005ef99f7edda215919ef5e . Which means the gst_mpegv_parse_process_sc () is accepting offset and packet.

Created attachment 249860 [details] [review]
mpegvideoparse: remove the offset argument of  gst_mpegv_parse_process_sc()

Doesn't need to pass the offset of each GstMpegVideoPacket explicitly to
gst_mpegv_parse_process_sc() since we alreay passing GstMpegVideoPacket as
one argument.

@Gwenole: could you please explain what corner case you have seen?

Created attachment 249864 [details] [review]
mpegvideoparse: Add GstVideoCropMeta

If the encoded stream has any associated crop rectangle (SequenceDispalyExtension), then send it as GstVideoCropMeta to the downstream.

This patch is on top of my previous mpegvideometa related patches. I can create a new bug for this if needed.

I am going to do the followings stuffs:

-- fix slice paring in case of only one slice per frame
-- add an optimization based on comment 25, 30 and 35
-- create a new bug for Adding GstVideoMeta (on top of current git master)

Created attachment 250009 [details] [review]
mpegvideoparse: fix slice parsing in case of single slice per frame.

Created attachment 250047 [details] [review]
mpegvideoparse: optimise the usage of slice_info_array.

Review of attachment 249660 [details] [review]:

::: gst-libs/gst/codecparsers/gstmpegvideometa.h
@@ +99,3 @@
 				const GstMpegVideoPictureExt *pic_ext,
+				const GstMpegVideoQuantMatrixExt *quant_ext,
+				const GArray *slice_info_array

Why const since we want to g_array_ref() it? Though, it does not really matter. :)

Review of attachment 250047 [details] [review]:

::: gst/videoparsers/gstmpegvideoparse.c
@@ +622,3 @@
+    if (slice_index >= mpvparse->slice_info_array->len)
+      mpvparse->slice_info_array = g_array_set_size (mpvparse->slice_info_array,
+          mpvparse->slice_info_array->len + 1);

I don't think GArray works like a GList whereby you need to re-assign the output to the variable. :)

Another g_array_set_size() is missing elsewhere when you have completed to parse all slices to set the final GArray->len to the actual number of slices. This happens when you have less slices than the estimated amount of slices used to pre-allocate the array.

Unless I missed a slice_info_array->len assignment elsewhere?

(In reply to comment #46)
> Review of attachment 250047 [details] [review]:
> 
> ::: gst/videoparsers/gstmpegvideoparse.c
> @@ +622,3 @@
> +    if (slice_index >= mpvparse->slice_info_array->len)
> +      mpvparse->slice_info_array = g_array_set_size
> (mpvparse->slice_info_array,
> +          mpvparse->slice_info_array->len + 1);
> 
> I don't think GArray works like a GList whereby you need to re-assign the
> output to the variable. :)
> 
> Another g_array_set_size() is missing elsewhere when you have completed to
> parse all slices to set the final GArray->len to the actual number of slices.
> This happens when you have less slices than the estimated amount of slices used
> to pre-allocate the array.
> 
> Unless I missed a slice_info_array->len assignment elsewhere?

Besides, we need a way to identify holes in the slice_info_array, i.e. cases where we did not parse slice headers in the middle, that is missing info from the bitstream to begin with. Maybe add a "valid" field in GstMpegVideoSliceHdr struct?

Oh wait, what am I saying? I got confused by the slice_index var, thus implying you could somehow reconstruct an index into the slice_info_array.

@Gwenole,

The g_array_set_size() will increase the size and setting the array size to the new length correctly. So i guess we doesn't need to set it manually. Because for each frame, we need to free all the members of the array (irrespective of whether we parsed the slice information to all members or not).

Do i need any change anything other than the "const" declaration ? (I am confused with your comment 48 :) )

Created attachment 250094 [details] [review]
mpegvideometa: Add slice information to the GstMpegVideoMeta

mpegvideometa: Add slice information to the GstMpegVideoMeta (Remove the "const" type of GArray.)

(In reply to comment #49)
> @Gwenole,
> 
> The g_array_set_size() will increase the size and setting the array size to the
> new length correctly. So i guess we doesn't need to set it manually. Because
> for each frame, we need to free all the members of the array (irrespective of
> whether we parsed the slice information to all members or not).

I thought you wanted to use Edward's trick to fill in entries as you parse them, only re-allocate the array when needed, and reset the correct length at the end?

> Do i need any change anything other than the "const" declaration ? (I am
> confused with your comment 48 :) )

Forget about it, I thought your slice_index was computed and meant to be the index into the slice_info_array. :)

(In reply to comment #51)
> (In reply to comment #49)
d to use Edward's trick to fill in entries as you parse
> them, only re-allocate the array when needed, and reset the correct length at
> the end?

The optimization patch is doing two things:

-- pre-allocate the array before each frame
-- avoid the memcpy introduced by g_array_append()

> 
> Forget about it, I thought your slice_index was computed and meant to be the
> index into the slice_info_array. :)

slice_index is indexing to the next index of the GArray to which we can parse the slice. The value of slice_index is calculated to index the array correctly for frame boundaries also.

I would have really appreciated if this whole discussion/work had happened in another bug (this one is about *adding* gstmeta to codeparsers, not about *improving* the exixsting ones).

There are other codecparsers which will need it (h264, vc1,...).

Please move all patches/discussions to a new bug properly title ("Add slice header information to ...."). Also try to make just two patches to review. One that adds support to gstcodecparsers, the other one to use it in the element.

Thankyou

Four out of Five patches are dealing with different thing and i likes to keep them as separate.  I can squash patch2(0002...) and patch4(0004..) if needed.

I suggest to change the titile of this bug to ("Add slice header information to ....") since most of the comments were about this Slice_Hdr stuffs. And we can create a new bug with title "[API] codecparsers: add GstMetas to pass parsing results downstream" . This would be much easier i guess. Anyway it is just a suggestion.

A new bug has been created to track the slice_hdr addition to MpegVideoMeta.
https://bugzilla.gnome.org/show_bug.cgi?id=704865

@Gwenole: Sorry i have squashed all the patches including "mpegvideoparse: remove the offset argument of gst_mpegv_parse_process_sc()" since bilboed was saying to do so :)

Someone please reject all the attached patches here since I have no permission it seems. All of those are tracked separately in #704865 & #733056 .

Hi,

(In reply to comment #58)
> Someone please reject all the attached patches here since I have no permission
> it seems. All of those are tracked separately in #704865 & #733056 .

From my experience, if you try to "Approve" or "Reject" a patch through the "Review" pane, it will fail with a perl error IIRC if you don't provide any comment. One trick I use however, is to change a patch status through its "Details", where it can work without providing any comment.

Please give it a try. If it still doesn't work, I would mark all patches as obsolete.

(In reply to comment #59)
> Hi,
> 
> (In reply to comment #58)
> > Someone please reject all the attached patches here since I have no permission
> > it seems. All of those are tracked separately in #704865 & #733056 .
> 
> From my experience, if you try to "Approve" or "Reject" a patch through the
> "Review" pane, it will fail with a perl error IIRC if you don't provide any
> comment. One trick I use however, is to change a patch status through its
> "Details", where it can work without providing any comment.
> 

Itz worked ! thanks :) 

> Please give it a try. If it still doesn't work, I would mark all patches as
> obsolete.

Can we close this bug as resolved-obsolete ???

No, it's not resolved or anything.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/84.