[Bug 796519] Add AV1 codec parser

[GStreamer (GNOME Bugzilla)]

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

Víctor Manuel Jáquez Leal <vjaquez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #373048|none                        |reviewed
             status|                            |

--- Comment #27 from Víctor Manuel Jáquez Leal <vjaquez at igalia.com> ---
Review of attachment 373048:
 --> (https://bugzilla.gnome.org/review?bug=796519&attachment=373048)

::: gst-libs/gst/codecparsers/gstav1parser.c
@@ +39,3 @@
+#include "gstav1parser.h"
+
+#define GST_AV1_DEBUG_HELPER() GST_DEBUG("entering function %s", __func__)

IMHO, there's no need to print __func__ because GST_DEBUG already does that for
you. By default it show file name, line number and function name.
So basically you should print "enter".

Nevertheless, this is cool for development, not for merging :D

@@ +67,3 @@
+#ifdef __GNUC__
+  /*GNU C allows using compound statements as expressions,thus returning
calling function on error is possible */
+#define gst_av1_read_bits(br, nbits) ({GstAV1ParserResult _br_retval; guint64
_br_bits=gst_av1_read_bits_checked(br,(nbits),&_br_retval);
GST_AV1_EVAL_RETVAL_LOGGED(_br_retval); _br_bits;})

This macro is hard to follow and prone to error

Check https://webassemblycode.com/while0-macros-use/

It should be like

#define GST_AV1_READ_BITS(br, nbits) \
  do { \
    GstAV1ParserResult _br_retval; \
    guint64 br_bits = gst_av1_read_bits_checked(br, (nbits), &_br_retval); \
    GST_AV1_EVAL_RETVAL_LOGGED(_br_retval); \
    _br_bit; \
  } while(0)

Though, I don't know if that actually works in this returning value mechanism.

@@ +154,3 @@
+    return x;
+  return y;
+}

glib already offers MAX macro:

https://developer.gnome.org/glib/stable/glib-Standard-Macros.html#MAX:CAPS

Also MIN

@@ +173,3 @@
+
+  return z;
+}

This looks like CLAMP

https://developer.gnome.org/glib/stable/glib-Standard-Macros.html#CLAMP:CAPS

@@ +600,3 @@
+
+  if (seq_header->reduced_still_picture_header) {
+    seq_header->timing_info_present_flag = 0;

If the structure is already initialized by 0, it doesn't worth to set zero
particular variables.

Another option would be, not to initialize the structure, but fill it
completely, variable by variable. The issue would be if in the future a new
variable is added into the structure.

::: gst-libs/gst/codecparsers/gstav1parser.h
@@ +49,3 @@
+G_BEGIN_DECLS
+
+#define GUINT32_MAX UINT32_MAX

There is already G_MAXUINT32


https://developer.gnome.org/glib/stable/glib-Basic-Types.html#G-MAXUINT32:CAPS

@@ +130,3 @@
+ *
+ */
+

If I recall correctly, this space will break the gtkdoc generation...

It is a little bit complex, but it would be nice if you could check the correct
generation of the documentation.

@@ +1624,3 @@
+
+GST_CODEC_PARSERS_API
+GstAV1ParserResult gst_av1_parse_obu_header (GstAV1Parser * parser,
GstBitReader * br, GstAV1OBUHeader * obu_header, gsize annexb_sz);

I GstAV1Parser have to be created in the heap, it can contain its GstBitReader,
isn't it?

In that way, if I understand correctly, there wouldn't be no need to pass it
around as a parameter.

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