GNOME Bugzilla – Bug 771650
Adding additional tags to FLV header
Last modified: 2018-11-03 15:11:38 UTC
Created attachment 335848 [details] [review] Newly added tags Few CDNs recommend to have following tags in FLV headers, in rtmp stream. • videocodecid • audiocodecid • audioonly • videoonly • stereo • width • height • videodatarate • audiodatarate • audiosamplerate • audiosamplesize • audiochannels • framerate • avcprofile • avclevel • aacaot But current flvmux do not have few of them. I have added missing tags, and validated it. Seems to be working, please find patch in the attachment. Can some one review the patch ?? One can use following pipeline for testing: gst-launch-1.0 videotestsrc is-live=1 ! video/x-raw,width=176,height=144,framerate=25/1 ! x264enc ! flvmux streamable=true name=mux ! fakesink audiotestsrc ! audio/x-raw,rate=48000,channels=2,format="S16LE" ! voaacenc bitrate=24000 ! mux. --gst-debug=*flv*:5
Review of attachment 335848 [details] [review]: Does not show up well as a patch, generate you patch using "git format-patch" and resubmit.
(In reply to Nicolas Dufresne (stormer) from comment #1) > Review of attachment 335848 [details] [review] [review]: > > Does not show up well as a patch, generate you patch using "git > format-patch" and resubmit. I have attached patch with git format-patch format.
Created attachment 335892 [details] [review] New tags added in FLV mux
Review of attachment 335892 [details] [review]: ::: 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.
Created attachment 335974 [details] [review] Patch generated as per review comments Hi Nicolas, Code is modified as per the review comments. Removed codec parsing code. Instead level and profile information is read from pad template. To get video information video_info_from_caps() is called and parameters are picked from it. I have modified few of existing tags also to read parameter from VideoInfo instead of pad structure. Unfortunately audio_info_from_caps() works only with "audio/x-raw". So All the information is read form caps structure. ~ Sudhir
Hi Nicolas, Can you please review the patch.
Just a note, you should mark patch as being patches, and mark old one as obsolete, that will help.
Review of attachment 335974 [details] [review]: ::: gst/flv/gstflvmux.c @@ +861,3 @@ s = gst_caps_get_structure (caps, 0); + if (gst_video_info_from_caps (&info, caps)) { + if (info.width > 0) { Indentation seems wrong here. Maybe it would still be cleaner to check with gst_structure_has_field() ? @@ +918,3 @@ + gst_caps_unref (caps); + + /* AVC onMeatadata holds good only for h264 encoder Bad english, please fix. @@ +942,3 @@ + !g_strcmp0(profile, "baseline")) + idc = 66; + else if (!g_strcmp0(profile, "main")) Indentation is also not correct. Did you disable gst-indent ? @@ +1001,3 @@ + + tmp = gst_flv_mux_create_number_script_value ("audiodatarate", + cpad->bitrate / 1024); Script value is a gdouble, you are doing an integer division which will loose precision.
Created attachment 338573 [details] [review] Indentation corrected Hello Nicolas Dufresne, Sorry for the delay. gst_structure_has_field check is added when required. Indentation is corrected. I have run gst-indent. tmp = gst_flv_mux_create_number_script_value ("audiodatarate", cpad->bitrate / 1024.0); gdouble value is used in denominator so no precision losse. ~ Sudhir
Review of attachment 338573 [details] [review]: Looks good now. My only concern, and I'd like some feedfback before merging, this make the muxer/demuxer asymmetric. We should probably also add the bits in the demuxer to parse these. At the same occasion, the magic numbers added to be put in a common header (there is already a lot of magic numbers in this code, it's not great to add more).
I don't understand the addition of gst_structure_has_field() just before gst_structure_get_*() - I don't think those are needed? Also, when I read the last patch I thought this whole level/profile stuff was a bit silly - it can trivially get this information (codec profile/level idc) from the h264 codec_data in the input caps, since flvmux requires avc - but it should be possible to do this with fewer lines of code than in the initially proposed patch :)
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/298.