[Bug 796519] Add AV1 codec parser

[GStreamer (GNOME Bugzilla)]

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

--- Comment #45 from XuGuangxin <guangxin.xu at intel.com> ---
Review of attachment 373286:
 --> (https://bugzilla.gnome.org/review?bug=796519&attachment=373286)

::: gst-libs/gst/codecparsers/gstav1parser.c
@@ +558,3 @@
+ */
+GstAV1ParserResult
+gst_av1_parse_get_first_obu (GstAV1Parser * parser,

gst_av1_parse_get_first_obu and gst_av1_parse_get_next_obu are standalone
functions. It's not related to parser.

Could we split it into a different structure? For example:

GstAV1Split* gst_av1_split_new(data, size, use_annexb);
gst_av1_split_get_next_obu
gst_av1_split_free

@@ +1135,3 @@
+      break;
+    default:
+      return GST_AV1_PARSER_ERROR;

for a decoder/parser, if we meet unrecognized things. We usually drop it. This
is for forwarding compilability.
For example, av1 add new meta, but it's not important to us. Instead of return
error, we'd better ignore it

@@ +1157,3 @@
+  /* superres_params() */
+  if (seq_header->enable_superres) {
+    frame_header->use_superres = gst_av1_read_bit (br);

please make use_superres as a local variable

@@ +1216,3 @@
+{
+  GST_AV1_DEBUG ();
+  frame_header->render_and_frame_size_different = gst_av1_read_bit (br);

should be local

@@ +1240,3 @@
+  GST_AV1_DEBUG ();
+  for (i = 0; i < GST_AV1_REFS_PER_FRAME; i++) {
+    frame_header->found_ref = gst_av1_read_bit (br);

this is another example we can put it in local.
nobody use found_ref beyond the function.
Its only be used to init some other values

@@ +2629,3 @@
+
+static GstAV1ParserResult
+gst_av1_load_reference_frame (GstAV1Parser * parser)

could we change it to a more descriptive name.
like gst_av1_copy_info_from_reference

@@ +2647,3 @@
+    return GST_AV1_PARSER_ERROR;
+  frame_header->current_frame_id =
+      ref_info->entry[frame_header->frame_to_show_map_idx].ref_frame_id;

conside use 
ref = & ref_info->entry[frame_header->frame_to_show_map_idx]
and
ref->ref_frame_id

@@ +2789,3 @@
+    coded_tile_data_ptr = obu->data + gst_bit_reader_get_pos (&br) / 8;
+    memcpy (tile_list->entry[tile].coded_tile_data, coded_tile_data_ptr,
+        tile_list->entry[tile].tile_data_size_minus_1 + 1);

the max value of tile_data_size_minus_1 2^16, it's very expensive, could we
just record offset here. so we do not need memcpy

@@ +2861,3 @@
+  for (tile_num = tile_group->tg_start; tile_num <= tile_group->tg_end;
+      tile_num++) {
+    tile_group->entry[tile_num].tile_row =

uses
    entry = &tile_group->entry[tile_num]
    info = frame_header->tile_info
will simpify the codde

@@ +2870,3 @@
+    } else {
+      gint tile_size_minus_1 = gst_av1_bitstreamfn_le (&br,
+          frame_header->tile_info.tile_size_bytes, &retval);

consider return error if 

tile_group->entry[tile_num].tile_size - frame_header->tile_info.tile_size_bytes
> sz

::: gst-libs/gst/codecparsers/gstav1parser.h
@@ +1460,3 @@
+  gboolean allow_warped_motion;
+  gboolean reduced_tx_set;
+  gboolean is_filter_switchable;

better change this as a functional level temporary variable.
A decoder will not use it since they have interpolation_filter.

A basic idea is if the variable only used by one function, we can put it in
local.

@@ +1670,3 @@
+  GstAV1ReferenceFrameInfo ref_info;
+  /*References */
+  GstAV1SequenceHeaderOBU *seq_header;

Point to an external structure is harmful. You do not know allocation status of
the memory.  It may be freed by the user.

Why we need this and "gst_av1_parse_sequence_header_obu (GstAV1Parser * parser,
GstAV1OBU * obu, GstAV1SequenceHeaderOBU * seq_header);" at same time?
We'd better choose one side.
1. change this to "GstAV1SequenceHeaderOBU seq_header;" and remove seq_headder
in gst_av1_parse_sequence_header_obu
or.
2. keep seq_header in gst_av1_parse_sequence_header_obu, and pass  seq_header
to gst_av1_parse_frame_header_obu

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