[Bug 771650] Adding additional tags to FLV header

[GStreamer (GNOME Bugzilla)]

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

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

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

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

::: gst/flv/gstflvmux.c
@@ +915,3 @@
+      
+      /*AVC onMeatadata hols good only for h264 encoder 
+      Profile information is not present in */

Coding style for multi-line comment is:

/* Blabla bla ... 80
 * blabla 
 */

@@ +917,3 @@
+      Profile information is not present in */
+      if (g_strcmp0 (gst_structure_get_name (s), "video/x-h264") == 0)
+      {

Coding style is to set the scope opening on same line.

@@ +918,3 @@
+      if (g_strcmp0 (gst_structure_get_name (s), "video/x-h264") == 0)
+      {
+        if(cpad->video_codec_data){

Space before if, and after ).

@@ +925,3 @@
+          gst_buffer_map (cpad->video_codec_data, &s_info, GST_MAP_READ);
+          gst_buffer_unmap(cpad->video_codec_data, &s_info); 
+          reader = gst_byte_reader_new (s_info.data, s_info.size);

gst_buffer_map() return value is ignored, data is being accessed after unmap.

@@ +927,3 @@
+          reader = gst_byte_reader_new (s_info.data, s_info.size);
+         
+          gst_byte_reader_init (reader, s_info.data, s_info.size);

This call is for static byte reader, you call _new() or _init(), not both.

@@ +936,3 @@
+            GST_DEBUG_OBJECT (mux, "putting avcprofile %u in the metadata",
idc);
+            tmp = gst_flv_mux_create_number_script_value ("avcprofile", idc); 
+            script_tag = gst_buffer_append (script_tag, tmp);

Overall, I don't think parsing the codec data make sense in a muxer. This
should already be handled by h264parse already if you add the profile field
into your template caps.

@@ +1011,3 @@
+      
+      info = 0;
+      if(gst_audio_info_from_caps (&audio_info, caps)){

You should probably just call audio_info_from_caps() at the start, and pick all
the above information from it.

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