After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 734093 - vaapidecode: allow for per-codec element split
vaapidecode: allow for per-codec element split
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
git master
Other Linux
: Normal enhancement
: 1.9.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 724352 766206 768899
 
 
Reported: 2014-08-01 04:06 UTC by Gwenole Beauchesne
Modified: 2016-09-07 13:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
vaapidecode: register decoder with internal GType (6.53 KB, patch)
2016-03-09 19:34 UTC, Víctor Manuel Jáquez Leal
none Details | Review
vaapidecode: split out jpeg decoder (5.09 KB, patch)
2016-03-09 19:34 UTC, Víctor Manuel Jáquez Leal
none Details | Review
vaapidecodebin: don't handle jpeg decoding (884 bytes, patch)
2016-03-09 19:34 UTC, Víctor Manuel Jáquez Leal
none Details | Review
vaapidecode: register decoder with internal GType (4.68 KB, patch)
2016-03-10 13:14 UTC, Víctor Manuel Jáquez Leal
none Details | Review
vaapidecode: split out jpeg decoder (6.75 KB, patch)
2016-03-10 13:14 UTC, Víctor Manuel Jáquez Leal
none Details | Review
vaapidecodebin: don't handle jpeg decoding (1.45 KB, patch)
2016-03-10 13:14 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
vaapidecode: register decoder with internal GType (4.34 KB, patch)
2016-03-11 08:56 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
vaapidecode: split out jpeg decoder (5.73 KB, patch)
2016-03-11 08:56 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
vaapidecode: split all the codecs (3.82 KB, patch)
2016-06-29 11:16 UTC, Víctor Manuel Jáquez Leal
none Details | Review
vaapidecode: split all the codecs (3.39 KB, patch)
2016-07-14 10:56 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
vaapidecode: merge vc1 and wmv3 elements (1.29 KB, patch)
2016-09-06 10:37 UTC, Víctor Manuel Jáquez Leal
committed Details | Review

Description Gwenole Beauchesne 2014-08-01 04:06:23 UTC
Comply with the rest of GStreamer plug-in elements and split the all-in-one vaapidecode element into per-codec plug-in elements instead. i.e. follow the same element split that was done for vaapiencode element.

Additional goals: allow for easier per-codec pre-configuration, or additional work that would be needed.
Comment 1 Víctor Manuel Jáquez Leal 2016-02-02 10:40:32 UTC
I thinks this change is not useful anymore. For example, v4l2viddec, it is also a video decoder that handles all the possible codecs in the hardware. Perhaps we should rename vaapidecode to vaapidec, for sake of homogeneity.

In the case of the encoders, the story is different: we should rename them as vaapi{codec}enc
Comment 2 Tim-Philipp Müller 2016-02-29 09:22:42 UTC
This change might still be useful, e.g. if we want to give different decoders different ranks.

IMHO the JPEG decoder is currently not good/stable enough to be autoplugged, and its rank should be NONE accordingly, but we need to split the decoders in order to be able to advertise different ranks.
Comment 3 Sebastian Dröge (slomo) 2016-02-29 10:14:13 UTC
Yes, and I also think this should've happened for v4l2viddec.
Comment 4 sreerenj 2016-02-29 10:32:29 UTC
Side benefit: We can add individual authors name for each decoder :)
Comment 5 Nicolas Dufresne (ndufresne) 2016-02-29 15:22:57 UTC
I'm not clear on the use yet, at least in v4l2 atm, everything is bytestream, and there is no per-codec code. The decoder type is not a very nice way to rank decoders, it really depends on the HW.
Comment 6 Sebastian Dröge (slomo) 2016-02-29 15:37:29 UTC
With different decoder element factories you at least have the possibility to rank them differently for a specific hardware.
Comment 7 Nicolas Dufresne (ndufresne) 2016-02-29 15:54:27 UTC
It remains very theoretical, at least for V4L2. People don't include bad drivers in their kernels and the use case I've came across, they can't afford a SW fallback. I just don't agree it should have happened, it's not a generalization. I don't believe this should become a rule. It would have made absolutely no difference for anyone so far, except generating more code.
Comment 8 Sebastian Dröge (slomo) 2016-03-07 11:32:22 UTC
(In reply to Nicolas Dufresne (stormer) from comment #7)
> It remains very theoretical, at least for V4L2. People don't include bad
> drivers in their kernels

I don't think you can generalize like that. And there are going to be cases like with vaapi here.

> It would have made absolutely no difference for anyone so far,
> except generating more code.

Yes, only that now we can't ever change that if it ever makes a difference for someone because it would break API/ABI :)
Comment 9 Nicolas Dufresne (ndufresne) 2016-03-07 14:28:18 UTC
(In reply to Sebastian Dröge (slomo) from comment #8)
> > It would have made absolutely no difference for anyone so far,
> > except generating more code.
> 
> Yes, only that now we can't ever change that if it ever makes a difference
> for someone because it would break API/ABI :)

I don't know for vaapi, but with V4L2, yes you can. The element are automatically generated, their name are dynamic. It would be hard to change if we introduce the proposed set of element, the one we pass the device.
Comment 10 Tim-Philipp Müller 2016-03-07 14:31:03 UTC
Let's focus this discussion on vaapi only.

I think for vaapi the element split makes sense, and I think we should consider it for 1.8.
Comment 11 sreerenj 2016-03-07 16:18:26 UTC
(In reply to Tim-Philipp Müller from comment #10)
> Let's focus this discussion on vaapi only.
> 
> I think for vaapi the element split makes sense, and I think we should
> consider it for 1.8.

1.8, should be in this week, right ? :)

BTW, I strongly prefer to get it for 1.8 If we decide to proceed with separate decoder elements. First official release from upstream, new names already for the encoders, lets do the same for decoders, and avoid any confusion in future.
Comment 12 sreerenj 2016-03-07 16:29:08 UTC
(In reply to sreerenj from comment #11)
> (In reply to Tim-Philipp Müller from comment #10)
> > Let's focus this discussion on vaapi only.
> > 
> > I think for vaapi the element split makes sense, and I think we should
> > consider it for 1.8.
> 
> 1.8, should be in this week, right ? :)
> 
> BTW, I strongly prefer to get it for 1.8 If we decide to proceed with
> separate decoder elements. First official release from upstream, new names
> already for the encoders, lets do the same for decoders, and avoid any
> confusion in future.

But this might not be safe to do in short time, more changes needed in vaapidecodebin, need regression testing, etc.
Comment 13 Tim-Philipp Müller 2016-03-08 12:03:19 UTC
gst-vaapi 1.8.0 doesn't have to come out the exact same time as the rest of 1.8. We should try to figure out what's right and what we/you want.

Without a codec-split we should remove jpeg decoding as a feature entirely seeing that it's pretty broken across the board (any more sophisticated solution will probably take more time). We just can't expose a jpeg decoder that asserts or fails to decode many jpegs as a PRIMARY+N rank.
Comment 14 Víctor Manuel Jáquez Leal 2016-03-08 12:51:14 UTC
What about to split only jpegdec now and see how it goes, and continue later on.
Comment 15 Sebastian Dröge (slomo) 2016-03-08 13:01:50 UTC
You need the same infrastructure for all of them and can automate that. See how gst-omx, gst-libav, frei0r, etc are doing that.
Comment 16 Víctor Manuel Jáquez Leal 2016-03-09 19:34:06 UTC
Created attachment 323544 [details] [review]
vaapidecode: register decoder with internal GType

Don't expose the the vaapidecode GType, instead expose a function
which will register element.

This is the first step to split the decoder by codecs.
Comment 17 Víctor Manuel Jáquez Leal 2016-03-09 19:34:11 UTC
Created attachment 323545 [details] [review]
vaapidecode: split out jpeg decoder

Split, as a different element, the JPEG decoder.
Comment 18 Víctor Manuel Jáquez Leal 2016-03-09 19:34:17 UTC
Created attachment 323546 [details] [review]
vaapidecodebin: don't handle jpeg decoding

As JPEG decoder has been split and demoted, it cannot be handled by
vaapidecodebin
Comment 19 Víctor Manuel Jáquez Leal 2016-03-09 19:35:54 UTC
It was easier than I thought :) 

Still, I want to add a function gst_vaapi_codec_get_caps() in order to remove the caps_str in the codec map, hence we'll only need to pass the codec id as qdata.
Comment 20 Sebastian Dröge (slomo) 2016-03-10 08:55:09 UTC
Review of attachment 323544 [details] [review]:

::: gst/vaapi/gstvaapidecode.c
@@ +1085,3 @@
 static void
+gst_vaapidecode_base_init (GstVaapiDecodeClass * klass)
+{

If each type is registered individually like you do in the next patch, you can keep this in class_init. Each type has their own class init (which happens to be the same function) called.

base_init is only needed if you create deeper inheritance hierarchies.

@@ +1170,3 @@
+  };
+
+  type = g_type_from_name ("vaapidecode");

Type names are like GstVaapiDecode, not vaapidecode

@@ +1171,3 @@
+
+  type = g_type_from_name ("vaapidecode");
+  if (!type) {

Why could it be called multiple times?
Comment 21 Sebastian Dröge (slomo) 2016-03-10 08:56:18 UTC
Review of attachment 323545 [details] [review]:

::: gst/vaapi/gstvaapidecode.c
@@ +125,3 @@
+
+static const GstVaapiDecoderMap vaapi_decode_map[] = {
+#if USE_JPEG_DECODER

Why the #if?
Comment 22 Sebastian Dröge (slomo) 2016-03-10 08:57:24 UTC
Comment on attachment 323546 [details] [review]
vaapidecodebin: don't handle jpeg decoding

vaapidecodebin should probably at least get a FIXME comment to automatically autoplug the correct vaapidec_* element based on caps.
Comment 23 Víctor Manuel Jáquez Leal 2016-03-10 10:02:27 UTC
(In reply to Sebastian Dröge (slomo) from comment #21)
> Review of attachment 323545 [details] [review] [review]:
> 
> ::: gst/vaapi/gstvaapidecode.c
> @@ +125,3 @@
> +
> +static const GstVaapiDecoderMap vaapi_decode_map[] = {
> +#if USE_JPEG_DECODER
> 
> Why the #if?

Because there are VA-API versions (old ones) that don't provide JPEG decoding API. This is checked in the configure.ac.
Comment 24 Víctor Manuel Jáquez Leal 2016-03-10 10:08:26 UTC
Review of attachment 323544 [details] [review]:

::: gst/vaapi/gstvaapidecode.c
@@ +1171,3 @@
+
+  type = g_type_from_name ("vaapidecode");
+  if (!type) {

I cannot assure that gst_vaapidecode_register won't be called multiple times.
Comment 25 Víctor Manuel Jáquez Leal 2016-03-10 10:20:29 UTC
Also I need to modify gst_vaapidecode_ensure_allowed_caps()
Comment 26 Víctor Manuel Jáquez Leal 2016-03-10 13:14:29 UTC
Created attachment 323621 [details] [review]
vaapidecode: register decoder with internal GType

Don't expose the the vaapidecode GType, instead expose a function
which will register element.

This is the first step to split the decoder by codecs.
Comment 27 Víctor Manuel Jáquez Leal 2016-03-10 13:14:35 UTC
Created attachment 323622 [details] [review]
vaapidecode: split out jpeg decoder

Split, as a different element, the JPEG decoder.
Comment 28 Víctor Manuel Jáquez Leal 2016-03-10 13:14:41 UTC
Created attachment 323623 [details] [review]
vaapidecodebin: don't handle jpeg decoding

As JPEG decoder has been split and demoted, it cannot be handled by
vaapidecodebin

Added a fixme comment regarding the future removal of vaapidecode.
Comment 29 Víctor Manuel Jáquez Leal 2016-03-10 13:19:09 UTC
Changes:

vaapidecode: register decoder with internal GType

  * changed the type name GstVaapiDecode
  * gst_vaapidecode_base_init() is only a boilerplate

vaapidecode: split out jpeg decoder

  * added the codec name, since I figured out that we cannot rely on gst_vaapi_codec_get_name (for example divx) -- we cannot map GstVaapiCodec and the gstreamer element

vaapidecodebin: don't handle jpeg decoding

   * added the FIXME comment

Also I see that there is no need to change gst_vaapidecode_ensure_allowed_caps() since gst_video_decoder_proxy_getcaps() does the magic
Comment 30 Sebastian Dröge (slomo) 2016-03-10 18:03:09 UTC
Review of attachment 323621 [details] [review]:

::: gst/vaapi/gstvaapidecode.c
@@ +1156,3 @@
+  GTypeInfo typeinfo = {
+    sizeof (GstVaapiDecodeClass),
+    (GBaseInitFunc) gst_vaapidecode_base_init,

You can set this to NULL
Comment 31 Sebastian Dröge (slomo) 2016-03-10 18:04:38 UTC
Review of attachment 323622 [details] [review]:

::: gst/vaapi/gstvaapidecode.c
@@ +1091,3 @@
 static void
 gst_vaapidecode_base_init (GstVaapiDecodeClass * klass)
 {

This can all be in class_init ...

@@ +1210,3 @@
+    if (!type) {
+      /* create the gtype now */
+      type = g_type_register_static (GST_TYPE_VIDEO_DECODER, type_name,

... because this is GST_TYPE_VIDEO_DECODER for all and not GST_TYPE_VAAPI_DECODER for all but the base one
Comment 32 Víctor Manuel Jáquez Leal 2016-03-11 08:56:53 UTC
Created attachment 323687 [details] [review]
vaapidecode: register decoder with internal GType

Don't expose the the vaapidecode GType, instead expose a function
which will register element.

This is the first step to split the decoder by codecs.
Comment 33 Víctor Manuel Jáquez Leal 2016-03-11 08:56:59 UTC
Created attachment 323688 [details] [review]
vaapidecode: split out jpeg decoder

Split, as a different element, the JPEG decoder.
Comment 34 Víctor Manuel Jáquez Leal 2016-03-11 08:59:36 UTC
changelog: don't use base_init :)
Comment 35 Sebastian Dröge (slomo) 2016-03-11 09:02:03 UTC
Review of attachment 323688 [details] [review]:

Good to go, just minor memory leak

::: gst/vaapi/gstvaapidecode.c
@@ +1144,3 @@
   /* sink pad */
+  pad_template = gst_pad_template_new ("sink", GST_PAD_SINK, GST_PAD_ALWAYS,
+      gst_caps_from_string (map->caps_str));

The caps returned here need to be unreffed in theory... but are going to be alive all of the time the application runs anyway. Please fix for correctness
Comment 36 Víctor Manuel Jáquez Leal 2016-03-11 12:09:35 UTC
Attachment 323623 [details] pushed as 1b11e35 - vaapidecodebin: don't handle jpeg decoding
Attachment 323687 [details] pushed as 96ac9be - vaapidecode: register decoder with internal GType
Attachment 323688 [details] pushed as 1e1d3b1 - vaapidecode: split out jpeg decoder
Comment 37 Víctor Manuel Jáquez Leal 2016-03-11 12:11:51 UTC
commit 96ac9be - vaapidecode: register decoder with internal GType - has the fix for the memleak mentioned in comment 35

Leave the bug open because we need to continue with the rest of the codecs.
Comment 38 Víctor Manuel Jáquez Leal 2016-06-29 11:16:51 UTC
Created attachment 330559 [details] [review]
vaapidecode: split all the codecs

Split the vaapidecode to all the supported codecs with the format
vaapi{codec}dec.

Disable temporarly vaapidecodebin.
Comment 39 Víctor Manuel Jáquez Leal 2016-07-14 10:56:26 UTC
Created attachment 331485 [details] [review]
vaapidecode: split all the codecs

Split the vaapidecode to all the supported codecs with the format
vaapi{codec}dec.

vaapidecode is stil registered as a GObject type, but not as a
GStreamer feature, so it can be used internally by vaapidecodebin without
changing its code too much.
Comment 40 Víctor Manuel Jáquez Leal 2016-07-27 13:28:46 UTC
Attachment 331485 [details] pushed as 55daedf - vaapidecode: split all the codecs
Comment 41 Víctor Manuel Jáquez Leal 2016-08-02 08:26:15 UTC
Howdy!

Thought these patches are already landed, I have a question:

I have split wm3 and vc1 decoder in two different decoders: vaapiwm3dec and vaapivc1dec 

Was that a correct decision? Or is it better to have a single wm3 decoder?

@slomo, @tim, what do you think?
Comment 42 sreerenj 2016-08-02 09:40:31 UTC
wmv3 is the "window media video 9 simple and main profile" and wvc1 is the "windows media video 9 advanced profile". Don't know why libav chose separate decoder names though..

May be a new name "vaapiwmvdec" ?

my vote is for a single "vaapivc1dec" since vc1 is the open standard.
Comment 43 Víctor Manuel Jáquez Leal 2016-09-06 08:24:33 UTC
As discussed on IRC (2016-09-02):

13:05:06	ceyusa	the question is if we should merge wm3 and vc1 into a single element
13:05:13	ceyusa	or keep it split
13:05:16	slomo	are they always coming together? :)
13:06:18	sree_	:), why libav has separate elements?
13:06:53	slomo	they are different codec ids in ffmpeg
13:07:56	ceyusa	which is not the case in libva
13:08:16	slomo	then they should probably be the same element
13:08:27	slomo	iirc only one of the wmv3 profiles was compatible with vc1 though
13:08:41	__tim	maybe ad-n770 has an opinion :)
13:09:43	__tim	are there some subtle differences perhaps from before vc1 was standardised? I remember somewhere we had to pass through some extra 'fourcc' info so the decoder can do the right thing based on that
13:11:12	ad-n770	VC1 simple/main profiles are roughly the same as WMV3 Simple/Main profiles
13:11:20	sree_	wm3 == simple and main prfoile , wvc1 = advanced profile: All are supported in SMPTE as VC1 codec ... And AFAIK, wmva is what not supported in vc1
13:11:31	ad-n770	diferences come in the advanced profile
13:11:42	sree_	yup :)
13:12:05	sree_	WMVA is not supported by vaapi
13:12:30	ad-n770	yes, wmva is the issue, it's pre standard VC1 advanced profile
13:12:49	sree_	So I go for "vaapivc1dec" single element ,
13:13:49	ad-n770	I think this should be fine
13:13:54	ceyusa	ok
13:14:15	slomo	that's also what i remember, yes. i think gst-omx does the same
Comment 44 Víctor Manuel Jáquez Leal 2016-09-06 10:37:04 UTC
Created attachment 334889 [details] [review]
vaapidecode: merge vc1 and wmv3 elements

This patch merges vaapivc1dec and vaapiwmv3dec into a single
vaapivc1dec. Also, removed the WMVA format, since it is not
supported by libva.
Comment 45 Víctor Manuel Jáquez Leal 2016-09-07 13:27:52 UTC
Attachment 334889 [details] pushed as 5ed9670 - vaapidecode: merge vc1 and wmv3 elements