GNOME Bugzilla – Bug 734093
vaapidecode: allow for per-codec element split
Last modified: 2016-09-07 13:28:01 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.
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
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.
Yes, and I also think this should've happened for v4l2viddec.
Side benefit: We can add individual authors name for each decoder :)
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.
With different decoder element factories you at least have the possibility to rank them differently for a specific hardware.
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.
(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 :)
(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.
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.
(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.
(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.
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.
What about to split only jpegdec now and see how it goes, and continue later on.
You need the same infrastructure for all of them and can automate that. See how gst-omx, gst-libav, frei0r, etc are doing that.
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.
Created attachment 323545 [details] [review] vaapidecode: split out jpeg decoder Split, as a different element, the JPEG decoder.
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
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.
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?
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 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.
(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.
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.
Also I need to modify gst_vaapidecode_ensure_allowed_caps()
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.
Created attachment 323622 [details] [review] vaapidecode: split out jpeg decoder Split, as a different element, the JPEG decoder.
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.
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
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
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
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.
Created attachment 323688 [details] [review] vaapidecode: split out jpeg decoder Split, as a different element, the JPEG decoder.
changelog: don't use base_init :)
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
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
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.
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.
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.
Attachment 331485 [details] pushed as 55daedf - vaapidecode: split all the codecs
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?
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.
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
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.
Attachment 334889 [details] pushed as 5ed9670 - vaapidecode: merge vc1 and wmv3 elements