GNOME Bugzilla – Bug 723352
codecparsers_h264: Framerate estimation enhancement
Last modified: 2016-05-28 10:00:04 UTC
Created attachment 267713 [details] [review] codecparsers_h264: Framerate estimation POC When the h264 stream have VUI timing information and and the framerate is fixed, i think we could determine the framerate using: framerate = time_scale / num_units_in_tick / DeltaTfiDivisor / (field_pic_flag ? 2 : 1) It requires to have already parsed SPS (VUI info), SEI Pic timing message (if present) and a slice header. I used E2.1 section of h264 specification, especially the 'timing_info_present_flag', 'num_units_in_tick', 'time_scale' and 'fixed_frame_rate_flag' definitions. I attached a proof of concept, it just show how to calculate the framerate.
I think this makes sense. Can you provide an updated patch?
Created attachment 274089 [details] [review] H264 framerate estimatio proof of concept I updated proof of concept, it apply on master and it only show framerate calculation because, at this moment, I am not really sure how to add it to current parser since we need SPS, Pic timing (if present) and slice header. Hence I think it's better to have your point of view before going further. For now, I propose to remove current framerate calculation from gst_h264_parse_sps_data() (codecparser_h264). By doing this, we will only have the framerate in h264parse element after parsing a slice header. As a consequence, we will have to update caps each time we parse a slice header if framerate changed. Moreover, if we have frame_mbs_only flag equal to 0, we can have in the same sequence both frame and field (ie field_pic_flag equal to 0 and 1). So I am a little worried about instantaneous framerate changes. Also I noticed that we can have some issue when h264parse will be set to passthrough mode. Another way could be to make the method to estimate the framerate public and let the user choose when to update framerate.
I think moving the framerate estimation to the user (h264parse) instead of doing magic in the library sounds ok, ideally with some helper function that makes everybody's life easier.
Created attachment 276824 [details] [review] codecparsers_h264: add gst_h264_video_calculate_framerate() Add an helper to calculate framerate in h264 parser library.
Created attachment 276825 [details] [review] h264parse: use new gst_h264_video_calculate_framerate() to get framerate Use new function gst_h264_video_calculate_framerate() to update framerate. This patch has a dependency with #730363
Created attachment 276848 [details] [review] codecparsers_h264: add gst_h264_video_calculate_framerate() Fix documentation
In general these patches look good, but I think another patch that removes sps->fps_num and sps->fps_den (and the calculations related to them) would be nice :)
commit aeb6a520742fd82888e41ea23f9a39add51125a0 Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu May 22 16:12:01 2014 +0200 h264parser: Remove unused fps_num/fps_den fields Instead the newly added function should be used to calculate the framerate properly. commit 2c3e7b635279209a119c080ae4b40e599b009d91 Author: Aurélien Zanelli <aurelien.zanelli@parrot.com> Date: Mon May 19 17:52:38 2014 +0200 h264parse: use new gst_h264_video_calculate_framerate() to get framerate https://bugzilla.gnome.org/show_bug.cgi?id=723352 commit 753701bf689e2b349d382f79ba01f44d2e0e7602 Author: Aurélien Zanelli <aurelien.zanelli@parrot.com> Date: Mon May 19 17:43:54 2014 +0200 codecparsers_h264: add gst_h264_video_calculate_framerate() Add a new function to calculate video stream framerate which rely on SPS, slice header and pic timing using formula: time_scale 1 1 fps = ----------------- x --------------- x ------------------------ num_units_in_tick DeltaTfiDivisor (field_pic_flag ? 2 : 1) See section E2.1 of H264 specification for definition of variables. https://bugzilla.gnome.org/show_bug.cgi?id=723352
(In reply to comment #7) > In general these patches look good, but I think another patch that removes > sps->fps_num and sps->fps_den (and the calculations related to them) would be > nice :) Yes, since it was just a proposal, i didn't submit the clean of the other part. But I will do it. :)
Well I didn't see you did it.
This is an API break mss was using these member, so it broke the build.
So what should we do about this? I would prefer to break the API as it's in -bad and cleaner...
I think we could mark the old API as deprecated befoire removing it. About mss, since it doesn't use h264 library to parse stream, it could be quite difficult to get pic timing and slice header. However we can have the same result as before by calling gst_h264_video_calculate_framerate with field_pic_flag=0 and pic_struct=0. Maybe we should port mss to use h264 parser.
Created attachment 328484 [details] [review] smoothstreaming: update fps calculation for h264 codec parser API changes Use new gst_h264_video_calculate_framerate() API instead of fps_n/fps_d fields in SPS struct which are to be removed. Apparently H264 content in MSS is always non-interlaced/progressive, so we can just pass 0 for field_pic_flag and don't need to parse any slice headers first. https://msdn.microsoft.com/en-us/library/cc189080%28VS.95%29.aspx ---- I'm only 90% sure that page allows us to conclude things about MSS though, but one would suspect some other kind of external signalling for interlaced/non-interlaced in any case then.
Comment on attachment 328484 [details] [review] smoothstreaming: update fps calculation for h264 codec parser API changes Pushed this, don't see how it can possibly make anything worse. Only found one working sample stream with H264 but that one yields 0/1. commit 7d46d67c59eab6cfda24d43220dfba2b0f9131d3 Author: Tim-Philipp Müller <tim@centricular.com> Date: Wed May 25 10:59:00 2016 +0100 smoothstreaming: update fps calculation for h264 codec parser API changes Use new gst_h264_video_calculate_framerate() API instead of fps_n/fps_d fields in SPS struct which are to be removed. Apparently H264 content in MSS is always non-interlaced/progressive, so we can just pass 0 for field_pic_flag and don't need to parse any slice headers first if there's no external signalling. But even if that's not the case the new code is not worse than the existing code. https://msdn.microsoft.com/en-us/library/cc189080%28VS.95%29.aspx https://bugzilla.gnome.org/show_bug.cgi?id=723352
Let's try this again then: commit 1b58bbf3adc8745136a34506a5c92132349295c4 Author: Tim-Philipp Müller <tim@centricular.com> Date: Sat May 28 10:44:04 2016 +0100 h264parser: maintain minimal ABI compat Because we can. https://bugzilla.gnome.org/show_bug.cgi?id=723352 commit b5f8b556e916eeaa8586f7935caeef51d7fec5fc Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu May 22 16:12:01 2014 +0200 h264parser: Remove unused fps_num/fps_den fields Instead the newly added function should be used to calculate the framerate properly. https://bugzilla.gnome.org/show_bug.cgi?id=723352