After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 723352 - codecparsers_h264: Framerate estimation enhancement
codecparsers_h264: Framerate estimation enhancement
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.3.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 730363
Blocks:
 
 
Reported: 2014-01-31 10:16 UTC by Aurélien Zanelli
Modified: 2016-05-28 10:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
codecparsers_h264: Framerate estimation POC (3.28 KB, patch)
2014-01-31 10:16 UTC, Aurélien Zanelli
none Details | Review
H264 framerate estimatio proof of concept (4.15 KB, patch)
2014-04-11 12:44 UTC, Aurélien Zanelli
none Details | Review
codecparsers_h264: add gst_h264_video_calculate_framerate() (4.17 KB, patch)
2014-05-20 08:28 UTC, Aurélien Zanelli
none Details | Review
h264parse: use new gst_h264_video_calculate_framerate() to get framerate (2.28 KB, patch)
2014-05-20 08:31 UTC, Aurélien Zanelli
committed Details | Review
codecparsers_h264: add gst_h264_video_calculate_framerate() (4.76 KB, patch)
2014-05-20 11:48 UTC, Aurélien Zanelli
committed Details | Review
smoothstreaming: update fps calculation for h264 codec parser API changes (1.55 KB, patch)
2016-05-25 10:03 UTC, Tim-Philipp Müller
committed Details | Review

Description Aurélien Zanelli 2014-01-31 10:16:25 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.
Comment 1 Sebastian Dröge (slomo) 2014-04-10 07:08:40 UTC
I think this makes sense. Can you provide an updated patch?
Comment 2 Aurélien Zanelli 2014-04-11 12:44:41 UTC
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.
Comment 3 Sebastian Dröge (slomo) 2014-05-03 08:00:28 UTC
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.
Comment 4 Aurélien Zanelli 2014-05-20 08:28:51 UTC
Created attachment 276824 [details] [review]
codecparsers_h264: add gst_h264_video_calculate_framerate()

Add an helper to calculate framerate in h264 parser library.
Comment 5 Aurélien Zanelli 2014-05-20 08:31:21 UTC
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
Comment 6 Aurélien Zanelli 2014-05-20 11:48:06 UTC
Created attachment 276848 [details] [review]
codecparsers_h264: add gst_h264_video_calculate_framerate()

Fix documentation
Comment 7 Sebastian Dröge (slomo) 2014-05-22 14:10:37 UTC
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 :)
Comment 8 Sebastian Dröge (slomo) 2014-05-22 14:13:31 UTC
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
Comment 9 Aurélien Zanelli 2014-05-22 14:29:41 UTC
(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. :)
Comment 10 Aurélien Zanelli 2014-05-22 14:30:51 UTC
Well I didn't see you did it.
Comment 11 Nicolas Dufresne (ndufresne) 2014-05-22 15:42:19 UTC
This is an API break mss was using these member, so it broke the build.
Comment 12 Sebastian Dröge (slomo) 2014-05-26 07:22:29 UTC
So what should we do about this? I would prefer to break the API as it's in -bad and cleaner...
Comment 13 Aurélien Zanelli 2014-05-26 13:15:26 UTC
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.
Comment 14 Tim-Philipp Müller 2016-05-25 10:03:30 UTC
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 15 Tim-Philipp Müller 2016-05-28 09:31:48 UTC
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
Comment 16 Tim-Philipp Müller 2016-05-28 10:00:04 UTC
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