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 757597 - codecparsers: Add VP9 parser
codecparsers: Add VP9 parser
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.7.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 759192
 
 
Reported: 2015-11-04 16:44 UTC by sreerenj
Modified: 2015-12-30 10:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add vp9 parser (54.02 KB, patch)
2015-11-04 16:44 UTC, sreerenj
none Details | Review
add vp9 parser (54.29 KB, patch)
2015-11-05 11:55 UTC, sreerenj
none Details | Review
add VP9 codec parser (62.14 KB, patch)
2015-12-09 12:55 UTC, sreerenj
needs-work Details | Review

Description sreerenj 2015-11-04 16:44:42 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
Comment 1 sreerenj 2015-11-04 16:49:53 UTC
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 :)
Comment 2 Sebastian Dröge (slomo) 2015-11-04 21:23:22 UTC
Make private what should be private (and do the same with VP8 if it's exposing things that shouldn't be exposed).
Comment 3 sreerenj 2015-11-04 22:15:00 UTC
(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.
Comment 4 Sebastian Dröge (slomo) 2015-11-05 08:27:00 UTC
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.
Comment 5 sreerenj 2015-11-05 10:04:46 UTC
(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.
Comment 6 sreerenj 2015-11-05 11:55:49 UTC
Created attachment 314907 [details] [review]
add vp9 parser
Comment 7 sreerenj 2015-11-06 09:40:02 UTC
Any other feedback ? , going to push it in gstreamer-vaapi/codecparsers... :)
Comment 8 Sebastian Dröge (slomo) 2015-11-06 09:48:09 UTC
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.
Comment 9 sreerenj 2015-11-06 09:58:47 UTC
(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.
Comment 10 sreerenj 2015-12-09 12:55:30 UTC
Created attachment 317018 [details] [review]
add VP9 codec parser
Comment 11 sreerenj 2015-12-09 12:58:54 UTC
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...
Comment 12 sreerenj 2015-12-09 16:44:25 UTC
Is it okay if I push this?? Or someone wants to review it?
Comment 13 sreerenj 2015-12-30 05:47:33 UTC
I will be pushing this in the afternoon if there is no objection :)
Comment 14 Sebastian Dröge (slomo) 2015-12-30 09:04:44 UTC
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)
Comment 15 Sebastian Dröge (slomo) 2015-12-30 09:35:01 UTC
This was now pushed without considering the review comments. If you ask for objections please at least check your mails before.
Comment 16 sreerenj 2015-12-30 09:38:00 UTC
(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.
Comment 17 Sebastian Dröge (slomo) 2015-12-30 09:53:36 UTC
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.
Comment 18 Sebastian Dröge (slomo) 2015-12-30 09:53:52 UTC
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
Comment 19 Sebastian Dröge (slomo) 2015-12-30 09:56:18 UTC
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
Comment 20 sreerenj 2015-12-30 10:11:54 UTC
Sebastian, many thanks for fixing those !