Created attachment 332991 [details] [review]
0001-splitmuxsink-Add-option-to-split-at-exactly-max-size.patch

Actually, I suspect there is a bug when mux-overhead > 0, need to take that into account as well. But I'll do that together with whatever comes up from the review.

Review of attachment 332991 [details] [review]:

Thanks for the patch! Comments inline.

::: gst/multifile/gstsplitmuxsink.c
@@ +215,3 @@
+          "Split at exactly max-size-time ns. "
+          "Needs max-size-bytes to be 0 in order to be effective.",
+          DEFAULT_EXACT_TIME, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

I'm not sure about the naming here. It should be something that indicates splitmuxsink will request split points. "Split at exactly max-size-time" might not be possible if upstream ignores keyframe requests, or implements them incorrectly.

@@ +844,3 @@
   /* Store the overflow parameters as the basis for the next fragment */
+  splitmux->mux_start_time =
+      splitmux->muxed_out_time + splitmux->last_frame_duration;

Since splitmux->last_frame_duration might be GST_CLOCK_STIME_NONE if the buffer has no provided duration, we can only add it if valid.

@@ +948,1 @@
 

I'm not sure about removing the equality check here.

@@ +1711,3 @@
+            "Could not request a keyframe. Files may not split at the exact location they should");
+      break;
+    }

The output of splitmuxsink should be independent of repeated pausing/playing. This will request a new keyframe every time, which isn't right. If it's needed in order to generate the initial keyframe request, that should perhaps happen when the first data is released to the muxer.

Review of attachment 332991 [details] [review]:

Thanks for the patch! Comments inline.

::: gst/multifile/gstsplitmuxsink.c
@@ +215,3 @@
+          "Split at exactly max-size-time ns. "
+          "Needs max-size-bytes to be 0 in order to be effective.",
+          DEFAULT_EXACT_TIME, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

I'm not sure about the naming here. It should be something that indicates splitmuxsink will request split points. "Split at exactly max-size-time" might not be possible if upstream ignores keyframe requests, or implements them incorrectly.

@@ +844,3 @@
   /* Store the overflow parameters as the basis for the next fragment */
+  splitmux->mux_start_time =
+      splitmux->muxed_out_time + splitmux->last_frame_duration;

Since splitmux->last_frame_duration might be GST_CLOCK_STIME_NONE if the buffer has no provided duration, we can only add it if valid.

@@ +948,1 @@
 

I'm not sure about removing the equality check here.

@@ +1711,3 @@
+            "Could not request a keyframe. Files may not split at the exact location they should");
+      break;
+    }

The output of splitmuxsink should be independent of repeated pausing/playing. This will request a new keyframe every time, which isn't right. If it's needed in order to generate the initial keyframe request, that should perhaps happen when the first data is released to the muxer.

Why do we need a property for it anyway, couldn't splitmux just send the requests in any case, and if upstream doesn't support them so be it?

Created attachment 333010 [details] [review]
0001-splitmuxsink-Allow-time-and-bytes-to-reach-their-res.patch

Thanks for the review comments. After discussing on IRC, I am attaching the updated patches.

Created attachment 333011 [details] [review]
0002-splitmuxsink-Add-option-to-split-at-exactly-max-size.patch

Created attachment 333052 [details] [review]
0002-splitmuxsink-Add-option-to-split-at-exactly-max-size.patch

Works for me (my impression after the discussion on IRC was that there's a preference towards not doing this by default).

Thanks, committed!

commit b9a818870435afba579955c2c2f2c271f755bf72
Author: Vivia Nikolaidou <vivia@ahiru.eu>
Date:   Tue Aug 9 12:55:59 2016 +0300

    splitmuxsink: Add option to split at exactly max-size-time
    
    Will try to request a keyframe from the encoder to be sent at the target
    running time.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=769664

commit 369d37d227178bc4bb3c760d72e5403a3f11f261
Author: Vivia Nikolaidou <vivia@ahiru.eu>
Date:   Tue Aug 9 20:16:16 2016 +0300

    splitmuxsink: Allow time and bytes to reach their respective thresholds
    
    https://bugzilla.gnome.org/show_bug.cgi?id=769664