GNOME Bugzilla – Bug 757597
codecparsers: Add VP9 parser
Last modified: 2015-12-30 10:11:54 UTC
Created attachment 314840 [details] [review] add vp9 parser Adding vp9parser: -- Vp9 spec is still not open, so basically the code is based on libvpx -- So keeping the {vp9utils.PATENTS, LICENSE, AUTHORS} Similar to vp8 dboolhuff.{PATENS, LICENSE, AUTHORS} -- right now vp9utils only has quantization look-up tables, but if needed we can move more apis to this and still keep gstvp9parser.{h,c} under LGPL
There are many state variables which needs to be tracked across multiple frames. So kept the GstVp9ParserPrivate structure to track these. Another way to do this is: Align with vp8 parser, avoid GstVp9ParserPrivate and keep all tracking variable in GstVp9Parser struct. But this will expose many unnecessary state tacking variables to public structure. So I am still double minded :)
Make private what should be private (and do the same with VP8 if it's exposing things that shouldn't be exposed).
(In reply to Sebastian Dröge (slomo) from comment #2) > Make private what should be private (and do the same with VP8 if it's > exposing things that shouldn't be exposed). Okay, the current patch I have attached should be enough then.. There is slight difference from vp8 (for eg: Loopfilter adjustment stuffs), but the current patch is giving more clarity and much aligned with libvpx too.
So is there some private stuff to hide in the VP8 parser too? :) Note that you'll have some padding to the parser struct if you want to be able to extend it later because of the way how you implemented the private stuff. A better solution would be to allocate the private data *before* the public data, and just have a pointer back to it. Or just allocate it independently.
(In reply to Sebastian Dröge (slomo) from comment #4) > So is there some private stuff to hide in the VP8 parser too? :) Not really :) In gstreamerr-vaapi perspective the implementation is fine..But if you consider it from a codec-analyzer view point there are issues. For eg: if some fields are not available for a particular frame, they are copying from previous frame. That is enough for gst-vaapi or any other similar use cases. But if you really wanna print the values of fields only if exists in a frame(thats what analyzer is needed) then it won't work. BTW, vp9 has more tracking stuffs(eg: sizes of up to 8 reference frames)and I don't wanna expose those to public stuct. > Note that you'll have some padding to the parser struct if you want to be > able to extend it later because of the way how you implemented the private > stuff. A better solution would be to allocate the private data *before* the > public data, and just have a pointer back to it. Okay, It was like that initially. Then I thought itz better to avoid exposing even the pointer in public struct :) > Or just allocate it > independently.
Created attachment 314907 [details] [review] add vp9 parser
Any other feedback ? , going to push it in gstreamer-vaapi/codecparsers... :)
As you might notice there are 761 unreviewed patches in Bugzilla for GStreamer at this point, your patch is not even a week old at this point. Have some patience, and if you want to have patch reviews happen faster help with reviewing the other patches.
(In reply to Sebastian Dröge (slomo) from comment #8) > As you might notice there are 761 unreviewed patches in Bugzilla for > GStreamer at this point, your patch is not even a week old at this point. > Have some patience, and if you want to have patch reviews happen faster help > with reviewing the other patches. Ok, no problem,,, take your time :) Thinking to add super-frame parsing (vp9 has super-frames which is a container of more than one frame) and other couple of stuffs too. Meanwhile further patches will be pushed here: https://github.com/01org/gstreamer-codecparsers thanks.
Created attachment 317018 [details] [review] add VP9 codec parser
Would be great if we can get it in upstream..It will help to port the gstreamer-vaapi to upstream too..I think it is in a good shape. Tested over more than 100 streams...
Is it okay if I push this?? Or someone wants to review it?
I will be pushing this in the afternoon if there is no objection :)
Review of attachment 317018 [details] [review]: Just some minor things, please fix before merging ::: gst-libs/gst/codecparsers/Makefile.am @@ +19,3 @@ gstjpegparser.h \ + gstmpegvideometa.h \ + gstvp9parser.h vp9utils.h vp9utils.h should probably be in noinst_HEADERS ::: gst-libs/gst/codecparsers/gstvp9parser.h @@ +89,3 @@ + * @GST_VP9_PROFILE_1: Profile 1, 8-bit 4:4:4, 4:2:2, and 4:4:0. + * @GST_VP9_PROFILE_2: Profile 2, 10-bit and 12-bit color only, with 4:2:0 sampling. + * @GST_VP9_PROFILE_3: Profile 3, 10-bit and 12-bit color only, with 4:2:2/4:4:4/4:4:0 sampling. Would be nice to also add support for all these profiles to vp9enc/dec :) Do you know of sample streams? ::: gst-libs/gst/codecparsers/vp9utils.h @@ +21,3 @@ +int16_t vp9_dc_quant(int qindex, int delta, int bit_depth); + +int16_t vp9_ac_quant(int qindex, int delta, int bit_depth); These 3 functions can easily cause symbol conflicts. Rename them please, add a gst_vp9 prefix or whatever. Bonus points for adding G_GNUC_INTERNAL here (and for vp8utils)
This was now pushed without considering the review comments. If you ask for objections please at least check your mails before.
(In reply to Sebastian Dröge (slomo) from comment #15) > This was now pushed without considering the review comments. If you ask for > objections please at least check your mails before. Aha , collision :(, just pushed and saw your comment while trying to close... Will fix those, sorry.
commit 1bf448cbbd5ea0675f8b68708be658a20200d03b Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Dec 30 11:38:14 2015 +0200 vp9parser: Fix indentation to make gst-indent happier commit 45f2ad952f6ce065bb59c0805bde2f4c6a2382cd Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Dec 30 11:36:45 2015 +0200 vp9parser: Rename symbols to prevent symbol conflicts Also make clamp() a static function for the same reason and use CLAMP (as defined by GLib) in the GStreamer code.
commit b245e0f16c63c798da3ae3f5a331fdc27c5dcb31 Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Wed Dec 30 11:19:33 2015 +0200 codecparsers: Add VP9 codec parser https://bugzilla.gnome.org/show_bug.cgi?id=757597
commit f12b2a74569878da6c298c4e9f49e021954edfcb Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Dec 30 11:54:25 2015 +0200 win32: Update libgstcodecparsers.def with the new symbols
Sebastian, many thanks for fixing those !