Created attachment 266957 [details] [review]
0001-vp8parser-add-range-decoder-for-vp8

Created attachment 266958 [details] [review]
0002-vp8parser-consider-Base-2-only-for-range-decoder

Created attachment 266959 [details] [review]
0003-vp8parser-optimize-normalize-of-range-decoder

Created attachment 266960 [details] [review]
0004-vp8parser-refill-range-decoder-when-count-0-in-gst_v

Created attachment 266961 [details] [review]
0005-vp8parser-add-vp8-parser-files

Created attachment 266962 [details] [review]
0006-vp8parser-add-vp8-parser-test

Created attachment 267015 [details] [review]
0001-codec-parser-add-vp8-parser

Review of attachment 267015 [details] [review]:

A parser element based on this would be good to have :) Also do you plan the same for VP9 too?

::: gst-libs/gst/codecparsers/gstvp8parser.c
@@ +34,3 @@
+#include "string.h"
+
+// $13.4

no // comments, and some explanation what this number means (section of the spec) might be useful :)

@@ +644,3 @@
+
+/**
+ * gst_vp8_parse:

parse_frame_header

@@ +664,3 @@
+  guint16 tmp_16;
+  gint pos, i, j;
+

You might want to init the frame_hdr with 0 in the beginning

@@ +700,3 @@
+  pos = gst_byte_reader_get_pos (&Br);
+  range_decoder =
+      gst_vp8_range_decoder_new (data + pos, size - pos);

Or maybe you want the range decoder to be stack allocatable too? Much simpler

::: gst-libs/gst/codecparsers/gstvp8rangedecoder.c
@@ +24,3 @@
+#define GST_VP8_RD_VALUE_SIZE ((gint)sizeof(GstVP8RDValue)*CHAR_BIT)
+
+// The range is [RANGE_MIN, RANGE_MAX)

No // comments

@@ +86,3 @@
+    return NULL;
+
+  rd = g_new0 (GstRangeDecoder, 1);

g_slice_new0()

@@ +94,3 @@
+
+gint
+gst_vp8_range_decoder_get_prob (GstRangeDecoder * rd, guint8 probability)

These public functions all need some gtk-doc documentation... how they're supposed to be used, what they do. Or you make the range decoder private and don't install the header. But it might be interesting to use for other code.

::: tests/parser/vp8-parser.c
@@ +1,1 @@
+/* vp8-parser.c

Put this file in tests/icles I guess

Also please provide a real unit test for the parser, see the existing ones for h264 and others.

Created attachment 267110 [details] [review]
0001-codecparsers-add-vp8-parser

Created attachment 267111 [details] [review]
0002-tests-add-vp8parser-test-check

Created attachment 267112 [details] [review]
0003-tests-add-vp8-parser-test-in-icles

Review of attachment 267110 [details] [review]:

::: gst-libs/gst/codecparsers/gstvp8parser.c
@@ +650,3 @@
+GstVp8ParseResult
+gst_vp8_parse_frame_header (GstVp8FrameHdr * frame_hdr, const guint8 * data,
+    guint offset, gsize size)

Can we get the size of a frame too? Or do we depend on already properly framed data? Should be documented

::: gst-libs/gst/codecparsers/gstvp8parser.h
@@ +55,3 @@
+  GST_VP8_PARSER_NO_PACKET,
+  GST_VP8_PARSER_NO_PACKET_END,
+  GST_VP8_PARSER_ERROR,

Should we have these other enum values if only OK and ERROR can happen? Will the others be returned at a later point?

::: gst-libs/gst/codecparsers/gstvp8rangedecoder.c
@@ +175,3 @@
+ * @rd: the #GstRangeDecoder to read
+ *
+ *decode one bit basing on even probability (0x80)

Space missing

::: gst-libs/gst/codecparsers/gstvp8rangedecoder.h
@@ +53,3 @@
+
+gboolean
+gst_vp8_range_decoder_init (GstRangeDecoder * rd, const guint8 * buffer,

Namespace pollution. GstVp8RangeDecoder

Review of attachment 267111 [details] [review]:

::: tests/check/libs/vp8parser.c
@@ +82,3 @@
+};
+
+GST_START_TEST (test_vp8_parse)

Maybe add a test for non-keyframes too

@@ +91,3 @@
+
+  assert_equals_int (gst_vp8_parse_frame_header (&frame_hdr, vp8_frame_data, 0,
+    sizeof (vp8_frame_data)/sizeof (guint8)), GST_VP8_PARSER_OK);

sizeof(guint8) is always 1

Review of attachment 267112 [details] [review]:

::: tests/icles/vp8parser-test.c
@@ +1,1 @@
+/* vp8-parser.c

Maybe you can make this a simple GStreamer application that uses a "vp8parse" element that we don't have yet :) Otherwise looks ok

thanks slomo; patches have been update.

1. "init the frame_hdr with 0", 
there are two parts of parameter in frame_hdr, one is parsed from current frame; another is inherited from previous frame(multi_frame_data) which current frame may update them or may not. 
so, it's better: decoder init the frame_hdr to 0, then init multi_frame_data.

2. make range decoder to be stack allocatable.
now, I follow the GstByteReader implementation. 
vp8 parser needn't know the data structure of GstRangeDecoder.

3. what's the value to have a vp8 parser element?
vp8 frame size is always decided by its container (webm or ivf), there is no sync code in vp8 stream. when will a vp8 parser element is needed?

i don't have schedule for vp9 parser yet.

See comment 12 and comment 13, otherwise I think this is good to go.

(In reply to comment #15)
> thanks slomo; patches have been update.
> 
> 1. "init the frame_hdr with 0", 
> there are two parts of parameter in frame_hdr, one is parsed from current
> frame; another is inherited from previous frame(multi_frame_data) which current
> frame may update them or may not. 
> so, it's better: decoder init the frame_hdr to 0, then init multi_frame_data.

Should be documented but otherwise ok :)

> 3. what's the value to have a vp8 parser element?
> vp8 frame size is always decided by its container (webm or ivf), there is no
> sync code in vp8 stream. when will a vp8 parser element is needed?

Not sure when it would be needed, probably never... or for broken containers that don't mark keyframes properly, or for extracting further information from the VP8 bitstream (droppable frames, no-display frames, ...).

Created attachment 267209 [details] [review]
0001-codecparsers-add-vp8-parser

Created attachment 267210 [details] [review]
0002-tests-add-vp8parser-test-check

Created attachment 267211 [details] [review]
0003-tests-add-vp8-parser-test-in-icles

Comment on attachment 267209 [details] [review]
0001-codecparsers-add-vp8-parser

This should use the libvpx range decoder, like done in the VP8 RTP (de)payloaders:
http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/rtp/dboolhuff.h

Review of attachment 267211 [details] [review]:

::: tests/icles/Makefile.am
@@ +16,3 @@
+vp8parser_test_CFLAGS  = -I$(top_srcdir)/gst-libs $(GST_CFLAGS)
+vp8parser_test_LDADD   =
+vp8parser_test_LDFLAGS = $(GST_LIBS) $(top_builddir)/gst-libs/gst/codecparsers/libgstcodecparsers-@GST_API_VERSION@.la

The .la file should be in LDADD if I'm not mistaken

Created attachment 268623 [details] [review]
0003-tests-add-vp8-parser-test-in-icles

update vp8parser_test_LDFLAGS

(In reply to comment #20)
> (From update of attachment 267209 [details] [review])
> This should use the libvpx range decoder, like done in the VP8 RTP
> (de)payloaders:
> http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/rtp/dboolhuff.h

I had used libvpx range decoder before;
however, internal reviewer said Gst community may prefer a 'native' implementation of range decoder. then I asked someone to re-implement it -- the code we see now.

Do we have to switch to libvpx implementation now?

Yes, you also might want to talk to Gwenole about that.

When switching to the libvpx range decoder, please make it private API. Or if that is not possible create a GStreamer-style wrapper API around it.

gwenole:
should we switch back to libvpx range decoder?

Created attachment 270521 [details] [review]
0001-codecparsers-add-vp8-parser

update range decoder to the implementation in libvpx

Comment on attachment 270521 [details] [review]
0001-codecparsers-add-vp8-parser

Looks good, I'll merge that soon after some more testing.

Thanks for updating this

Review of attachment 268623 [details] [review]:

::: tests/icles/vp8parser-test.c
@@ +19,3 @@
+ * Boston, MA 02110-1301, USA.
+ */
+#include "gst/codecparsers/gstvp8parser.h"

Use <> for the include

@@ +22,3 @@
+#include <string.h>
+#include <stdio.h>
+#define PRINTF printf

Just use g_print() here :)

@@ +40,3 @@
+  guint32 time_scale;
+  guint32 num_frames;
+  // guint8  unused[4];

No C99 comments

@@ +110,3 @@
+
+error:
+  GST_WARNING ("failed in %s", __FUNCTION__);

I think these should all be g_warnings() or similar

@@ +302,3 @@
+  GstVp8RangeDecoderStatus *state = NULL;
+  gint frame_num = 0;
+

Some kind of "--help" or "--usage" style output would be good so that one knows what this is supposed to do and how it is used :)

Also put a comment about that at the top of the file

This is on hold for the time being, please do not push this.

Created attachment 273426 [details] [review]
libvpx: import upstream sources (version 1.3.0)

The original libvpx needs to be used, unmodified. So, here is a patch to let gst-plugins-bad track the upstream libvpx version 1.3.0, and build a suitable libgstcodecparsers_vpx.so library with all symbols moved to the GSTREAMER namespace. Only the required symbols are being exported and used. Since some of the definitions (tables) are available in headers only, and we don't want libgstcodecparsers to include those, some additional indirection was also needed.

Created attachment 273428 [details] [review]
codecparsers: add VP8 bitstream parser

This version now exclusively uses libvpx as a separate DSO. Some tidy up is further required, and additional bug fixes to come. Anyhow, this yields zero change vs. the previous patch, no new bug introduced, no older bug fixed either. :)

We will have to see if that's the path we want to go down. Please do not push at this point.

Created attachment 273429 [details] [review]
libvpx: import upstream sources (version 1.3.0)

Changes: fixed gstlibvpx.[ch] license to match libvpx license and have the resulting libgstcodecparsers_vpx.so fully BSD licensed.

(In reply to comment #32)
> We will have to see if that's the path we want to go down. Please do not push
> at this point.

No problem. Feel free to use whatever alternative suits you best. I will keep updating the core gstvp8parser.[ch] files to match libvpx APIs. Drop-in replacements are still possible, as previously mentioned.

Created attachment 273522 [details] [review]
codecparsers: add VP8 bitstream parser

Changes:
- More documentation
- Renamed a few things to better match the reality
- Fixed a couple of bugs, in particular when default values are to be used
- Calculated all partition sizes, including the last one
- Removed frame_data from frame header struct, this is an invitation to dangling pointers. Rather, pass down the persistent state as an additional argument to gst_vp8_parse_frame_hdr().

I am not fully satisfied yet of that version:
- Some minor improvements are needed for the range decoder state
- I would like to add a header_size in bits at the end of the frame_hdr too

There is also another patch to calculate PAR.

We should do the same as in the RTP VP8 payloader and just copy the sources of the arithmetic coding implementation in here, instead of creating another shared library. That just doesn't make sense.

Don't push this before this is changed.

Review of attachment 273522 [details] [review]:

Looks good except for what I wrote in the previous comment

(In reply to comment #37)
> Review of attachment 273522 [details] [review]:
> 
> Looks good except for what I wrote in the previous comment

Thanks, I would like to make an ultimate API change in a separate patch maybe. i.e. make the GstVp8FrameData structure the GstVp8Parser instead and move gst_vp8_frame_data_init() to gst_vp8_parser_init() + rename gst_vp8_parse_frame_hdr() to gst_vp8_parser_parse_frame_hdr().

Reason behind this: the parser is best suited to track the entropy coding probs itself, without assuming that the upper layer would make the additional updates to GstVp8FrameData when necessary. Besides, I would like GstVp8FrameHdr to fully represent the parsed state at a point in time, without depending on another struct. Here, I am thinking about a GstVp8Meta.

WDYT?

Created attachment 273788 [details] [review]
codecparsers: add VP8 bitstream parser

Changes:
- Added range decoder API to be separately implemented
- Added GstVp8ModeProbs structure for intra-prediction mode decoding tree
- Renamed GstVp8FrameData to GstVp8Parser as this is really parser context

Just noticed I will have to fix comments and add Since: tags while pushing, when accepted.

Created attachment 273789 [details] [review]
codecparsers: vp8: add GStreamer native utilities

Implement the VP8 range decoder and utilities per comment #36

Created attachment 273825 [details] [review]
codecparsers: add VP8 bitstream parser

Changes:
- Added "Since:" tags and fixed a couple of other comments
- Fixed gst_vp8_parser_init() to reset segmentation struct

Created attachment 273826 [details] [review]
codecparsers: vp8: add GStreamer native utilities

Changes:
- Added dboolhuff.{PATENTS,AUTHORS} as the license template references those
- Fixed Makefile.am (EXTRA_DIST, SOURCES) to actually include the new files

Comment on attachment 267210 [details] [review]
0002-tests-add-vp8parser-test-check

This needs to be updated for the API changes

Thanks Gwenole, I have merged your patches locally and did some minor changes... but the tests need to be updated to your API changes now. Can you do that? :)

(In reply to comment #44)
> Thanks Gwenole, I have merged your patches locally and did some minor
> changes... but the tests need to be updated to your API changes now. Can you do
> that? :)

I planned to fix that prior to pushing. What kind of changes did you make? Thanks.

Created attachment 273955 [details] [review]
0003-codecutils-Add-dboolhuff.-ch-to-the-build.patch

Created attachment 273956 [details] [review]
0004-codecparsers-Rename-VP8-dboolhuff-symbols-to-avoid-c.patch

Please push my patches together with yours when you fixed the tests.

Created attachment 274675 [details] [review]
tests: add test for VP8 bitstream parsing library

Changes:
- Updated for new parser API
- Made inter frame test independent from key frame test. i.e. parse frame0 again.

Created attachment 274676 [details] [review]
tests: add standalone program for VP8 parser

Changes:
- Updated to new parser API
- Fixed exit codes for both failure or success
- Fixed and simplified indentation of the output
- Addressed all comments except the --help part :)

This is only useful for debugging purposes. It might not be needed at all nowadays, but since it exists, just pushing it somewhere for posterity.

commit bf6959000f2f785c9446f32bf2bf8eb619633760
Author: Zhao, Halley <halley.zhao@intel.com>
Date:   Fri Jan 24 08:37:16 2014 +0800

    tests: add standalone program for VP8 parser.
    
    Add standalone test application that demonstrates how to use the new
    VP8 bitstream parsing library, while also allowing simple debugging/
    tracing of IVF files.
    
    [clean-ups, updated to new parser API]
    Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>

commit 26baaf51a1994f153eab88f691fda6c2b785ec6a
Author: Zhao, Halley <halley.zhao@intel.com>
Date:   Wed Jan 8 02:49:00 2014 +0800

    tests: add test for VP8 bitstream parsing library.
    
    [updated to new parser API]
    Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>

commit cea860021d093a699619a7903797a2af5d2a6784
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Apr 9 09:22:02 2014 +0200

    codecparsers: vp8: rename dboolhuff symbols.
    
    Rename VP8 dboolhuff symbols so that to avoid clashes with libvpx when
    static linking.

commit e2aaa91c5aa5fb45bec35c348fcef6e907d15efa
Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com>
Date:   Tue Apr 8 10:30:09 2014 +0200

    codecparsers: vp8: add GStreamer native utilities.
    
    Import libvpx 1.3.0 range decoder files (dboolhuff.[ch]) to implement
    the VP8 utilities native interface. Likewise, copy and use the default
    libvpx generated entropy probabilities tables.
    
    Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>

commit 3da5a543995542ffaa57f9e179b5b9fbe0bc850f
Author: Zhao, Halley <halley.zhao@intel.com>
Date:   Wed Jan 8 02:49:00 2014 +0800

    codecparsers: add VP8 bitstream parser.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=722760
    
    [refactored, among other fixes]
    Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>