[Bug 796519] Add AV1 codec parser

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=796519

Nicolas Dufresne (ndufresne) <nicolas at ndufresne.ca> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #372967|none                        |needs-work
             status|                            |

--- Comment #6 from Nicolas Dufresne (ndufresne) <nicolas at ndufresne.ca> ---
Review of attachment 372967:
 --> (https://bugzilla.gnome.org/review?bug=796519&attachment=372967)

Just a high level review. I agree with the comment that it's uncommon to expose
a BitReader in the API. You could have a context that holds one if needed.
Though, in general we use the GST_READ* macro as BitReader is slow (requires a
lot of function calls). I haven't looked at the code yet, and haven't looked at
the API.

Is AV1 like VP8/9, which cannot be frames from raw data ? Or is it more like
JPEG or H264 byte-stream ?

::: gst-libs/gst/codecparsers/gstav1parser.h
@@ +8,3 @@
+ * modify it under the terms of the GNU Library General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.

I think GNU Lesser 2.1 should be used in new code.

@@ +25,3 @@
+ */
+ /**
+  * SECTION:gstav1parser

This needs to be added to the doc sections in docs/

@@ +30,3 @@
+  *
+  * For more details about the structures, you can refer to the
+  * specifications: https://aomediacodec.github.io/av1-spec/av1-spec.pdf

Should be mark down annotation. Something like [specification](https://..)

@@ +31,3 @@
+  * For more details about the structures, you can refer to the
+  * specifications: https://aomediacodec.github.io/av1-spec/av1-spec.pdf
+  *

Add "Since: 1.16"

@@ +52,3 @@
+
+
+//TODO: Import all Defines from SPEC

Use C style comment, /* */

@@ +143,3 @@
+/**
+ * GstAV1OBUType:
+ *

Doc/TODO

@@ +389,3 @@
+
+typedef enum {
+  GST_AV1_REF_INTRA_FRAME,

All other enum used explicit =.

@@ +416,3 @@
+
+/**
+ * _GstAV1OBUHeader:

Remove _ here, you want to refer to the typedef, not the struct.

@@ +432,3 @@
+
+struct _GstAV1OBUHeader {
+  guint8 obu_forbidden_bit;

We don't usually put reserve bits into parsed structure.

@@ +485,3 @@
+ *                                             initial_display_delay_minus_1
is not specified for this
+ *                                             operating point.
+ * initial_display_delay_minus_1: plus 1 specifies, for operating point i, the
number of decoded frames

Missing @.

@@ +592,3 @@
+  guint8 separate_uv_delta_q;
+  guint8 BitDepth;
+  guint8 NumPlanes;

use underscore notation instead of camel case.

@@ +724,3 @@
+  guint8 itu_t_t35_country_code;
+  guint8 itu_t_t35_country_code_extention_byte;
+  // itu_t_t35_payload_bytes - not supported at the moment

Without structure padding, you'll need to add this in, as adding it later would
be an ABI break.

@@ +1134,3 @@
+  guint8 UsesLr;
+  GstAV1FrameRestorationType FrameRestorationType[GST_AV1_MAX_NUM_PLANES];
+  guint32 LoopRestorationSize[GST_AV1_MAX_NUM_PLANES];

No camel case please.

@@ +1552,3 @@
+    guint8 anchor_tile_col;
+    guint16 tile_data_size_minus_1;
+    //guint8 coded_tile_data[]; //skipped

Would need to reserve that space in the struct for ABI stability.

::: tests/check/meson.build
@@ +70,3 @@
 ]

+

Spurious new line.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.