Is this the same as bug #760505 ?

Created attachment 319248 [details] [review]
fix framerate calculation for fragmented format

This is related to bug #760505
However, this will work only pull mode. For push mode, additional work needed.

Review of attachment 319248 [details] [review]:

::: gst/isomp4/qtdemux.c
@@ +6944,3 @@
+        } else {
+          n_samples = stream->n_samples;
+          duration = stream->duration;

What if fragmented but duration or nsamples is unknown? Should we just not calculate anything at all here then and don't set any framerate in the caps?

Also should the framerate (avg duration) be updated whenever a new moof is arriving, and especially be updated in the caps too? I think this is not done here, or is it?

To simplify, let's focus on pull mode for this discussion. (push mode is being discussed in bug #760779)
There are 2 questions
- What if fragmented & duration and/or n_samples are unknown?
- Need update caps for every new moof? If so, where should be done?

Q: What if fragmented & duration and/or n_samples are unknown?
A: In my opinion, unknown duration and/or n_samples will not happen if the mp4 file is not broken.
And unknown duration and/or n_samples case is already covered by this codes
6917       if (stream->duration == 0 || stream->n_samples < 2) {
6918         stream->fps_n = stream->timescale;       
6919         stream->fps_d = 1;
6920       } else {
6921         /* Calculate a framerate, ignoring the first sample which is  

Let's assume that two fragmented case, And this patch is just for Case 2.
Case 1. moov + mdat_1 + moof_1 + mdat_2 + moof_2 + mdat_3 + ...
Case 2. moov + moof + mdat + moof + mdat + ....

For the Case 1, moov has n_sample data of mdat_1. So, sample data of moof_1 will not parsed 
in qtdemux_prepare_stream (In GST_STATE_HEADER/INITIAL), because n_samples is non-ZERO.
@qtdemux_prepare_streams (GstQTDemux * qtdemux)
..
10455     if (qtdemux->fragmented) {
10456       /* need all moov samples first */
10457       GST_OBJECT_LOCK (qtdemux);  
10458       while (stream->n_samples == 0)
10459         if ((ret = qtdemux_add_fragmented_samples (qtdemux)) != GST_FLOW_OK)  

Then, in this case, n_samples_moof and duration_moof has inital value (zero), 
so qtdemux will calculate framerate based on only available data in moov.
I think this is what qtdemux did without this proposed patch.

Let's take a look at Case 2. qtdemux try to parse moof's until find sample data (in the trun box), 
then frame rate will be calculated by the first valid moof.
So, it will also work properly.

Please note that, proposed patch will work by following condition
+        if (qtdemux->fragmented && stream->n_samples_moof > 0
+            && stream->duration_moof > 0) 

In summary, unknown duration and/or n_samples will not happen for case 2.


Q: Need update caps for every new moof? If so, where should be done?
A: I do not think that calcuates frame rate and update for every moof is very meaningful.
It imply that whenever jump from on moof to another, qtdemux need configure stream again (call gst_qtdemux_configure_stream() function).
Also, this patch did not consider this case. Note that, qtdemux configure stream again if it get new caps from sink pad.

For the first question, makes sense.


For the second, I think that would be useful to do as our framerate calculation is now based on only one fragment average and might be a bit off especially for a shorter first fragment. It might make sense to average it over multiple fragments. But best to do that as a new patch on top. Would you do that?

Created attachment 319912 [details] [review]
fix framerate calculation for fragmented format (patch set #2)

Differences between patch set #1 and #2 are that,

- Modification 1: Initialization point of "n_samples_moof" and "duration_moof" is changed from "per trun box" to "traf box", because multiple trun can exist in a moof. This modification is to accumulate samples in one moof.

- Modification 2: Frame rate will be re-calculated using new moof and caps also be updated by patch set #2.

Dear Sebastian Dröge,
I have two questions.
- Q1: Could you please clarify your comment "But best to do that as a new patch on top"? Does it means report new Bug? or implement it on this patch set?

- Q2: I had a rethink about the second issue, and I agree with your comment that short first fragment might cause incorrect frame rate calculation.
So, I modified frame rate to be updated per moof. However, based on your comment, you prefer averaging frame rate values over multiple fragments.

I think it is not difficult to accumulate n_samples and duration data over multiple moof. But I need your opinion on my following question.

- mp4 supports variable frame rate. That means, frame rate can be varied as time goes on. In this context, we need to choose "piecewise calculated frame rate" or "keep averaging frame rate". My choice is that "piecewise calculated frame rate" is better than the other (that's the reason why I implement "Modification 2"). Could you give me feedback?

(In reply to Seungha Yang from comment #8)
> - Q1: Could you please clarify your comment "But best to do that as a new
> patch on top"? Does it means report new Bug? or implement it on this patch
> set?

A separate patch, can be in this bug though.

> - Q2: I had a rethink about the second issue, and I agree with your comment
> that short first fragment might cause incorrect frame rate calculation.
> So, I modified frame rate to be updated per moof. However, based on your
> comment, you prefer averaging frame rate values over multiple fragments.
> 
> I think it is not difficult to accumulate n_samples and duration data over
> multiple moof. But I need your opinion on my following question.
> 
> - mp4 supports variable frame rate. That means, frame rate can be varied as
> time goes on. In this context, we need to choose "piecewise calculated frame
> rate" or "keep averaging frame rate". My choice is that "piecewise
> calculated frame rate" is better than the other (that's the reason why I
> implement "Modification 2"). Could you give me feedback?

Good point, I guess only averaging per moof makes sense then instead of using a running average or so. It's just that for the first fragment it might be a bit off but maybe that doesn't matter that much.

Dear Sebastian Dröge

Thanks for quick reply.

There might be a smart solution to handling that kind of short moof
e.g., 
- gathering more moof's (but it will cause delay and it looks ugly for me...). 
- do not set frame rate for that case.

Currently, I'm not sure how to deal with it. So, I think that it's better to separate patch to handle "short first fragment". And, my opinion about short first fragment also that "it may not cause big problem".

Let's go with that then and worry about it when it becomes a problem :)

(In reply to Sebastian Dröge (slomo) from comment #11)
> Let's go with that then and worry about it when it becomes a problem :)

I fully agree with your opinion.

Ok, can you update the patch? :)

Or was it already done except for the additional things for another patch?

commit 7873bede3134b15e5066e8d14e54d1f5054d2063
Author: Seungha Yang <sh.yang@lge.com>
Date:   Tue Jan 26 22:37:30 2016 +0900

    qtdemux: fix framerate calculation for fragmented format
    
    qtdemux calculates framerate using duration and the number of sample.
    In case of fragmented mp4 format, however, the number of sample can
    be figure out after parsing every moof box. Because qtdemux does not
    parse every moof in QTDEMUX_STATE_HEADER state, it will cause incorrect
    framerate calculation.
    
    This patch will triger gst_qtdemux_configure_stream() for every new moof.
    Then, framerate will be calculated by using duration and n_samples of the moof.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=760774