Hi Roman,

Apologies for a long delay. I was across the country and 50% time for 2 months and took a couple weeks to catchup…

Alright, your updated patch is looking good.
I think the overall name change from “mp4_exact_start” to “mp4_seek_key_frame” sounds fine to me.
I’ve compiled current head-of-master with your patch and tested on MacOSX and it’s looking to work the same as the prior patch, kudos!

I want to add some temporary debug lines to make sure I understand (especially) the way you cleverly avoided an nginx alloc for extra entry :)
and to test on linux.

Both of those should be pretty straightforward and anticipate no issues/concerns.

What sounds good for the next steps from your POV?

If you imagine I wouldn’t be getting commit/push rights and added as a contributor, I’d love to add next to my name (thank you for adding that) somewhere something like:
Tracey Jaquith, Internet Archive
Tracey Jaquith tracey@archive.org <mailto:tracey@archive.org>

Since I worked on this primarily for my job purposes and I’d love the idea that both myself and the Archive are porting upstream and idea, code, etc.


Very appreciatively!
-Tracey


> On Jun 28, 2021, at 2:53 AM, Roman Arutyunyan <arut@nginx.com> wrote:
>
> Hi Tracey,
>
> On Tue, Jun 15, 2021 at 03:49:48PM -0700, Tracey Jaquith wrote:
>> # HG changeset patch
>> # User Tracey Jaquith <tracey@archive.org>
>> # Date 1623797180 0
>> # Tue Jun 15 22:46:20 2021 +0000
>> # Node ID 1879d49fe0cf739f48287b5a38a83d3a1adab939
>> # Parent 5f765427c17ac8cf753967387562201cf4f78dc4
>> Add optional "mp4_exact_start" nginx config off/on to show video between keyframes.
>
> I've been thinking about a better name for this, but came up with nothing so
> far. I feel like this name does not give the right clue to the user.
> Moreover, when this feature is on, the start is not quite "exact", but shifted
> a few milliseconds into the past.
>
>> archive.org has been using mod_h264_streaming with a similar "exact start" patch from me since 2013.
>> We just moved to nginx mp4 module and are using this patch.
>> The technique is to find the video keyframe just before the desired "start" time, and send
>> that down the wire so video playback can start immediately.
>> Next calculate how many video samples are between the keyframe and desired "start" time
>> and update the STTS atom where those samples move the duration from (typically) 1001 to 1.
>> This way, initial unwanted video frames play at ~1/30,000s -- so visually the
>> video & audio start playing immediately.
>>
>> You can see an example before/after here (nginx binary built with mp4 module + patch):
>>
>> https://pi.archive.org/0/items/CSPAN_20160425_022500_2011_White_House_Correspondents_Dinner.mp4?start=12&end=30
>> https://pi.archive.org/0/items/CSPAN_20160425_022500_2011_White_House_Correspondents_Dinner.mp4?start=12&end=30&exact=1
>>
>> Tested on linux and macosx.
>>
>> (this is me: https://github.com/traceypooh )
>
> We have a few rules about patches and commit messages like 67-character limit
> for the first line etc:
>
> http://nginx.org/en/docs/contributing_changes.html
>
>> diff -r 5f765427c17a -r 1879d49fe0cf src/http/modules/ngx_http_mp4_module.c
>> --- a/src/http/modules/ngx_http_mp4_module.c Tue Jun 01 17:37:51 2021 +0300
>> +++ b/src/http/modules/ngx_http_mp4_module.c Tue Jun 15 22:46:20 2021 +0000
>> @@ -43,6 +43,7 @@
>> typedef struct {
>> size_t buffer_size;
>> size_t max_buffer_size;
>> + ngx_flag_t exact_start;
>> } ngx_http_mp4_conf_t;
>>
>>
>> @@ -340,6 +341,13 @@
>> offsetof(ngx_http_mp4_conf_t, max_buffer_size),
>> NULL },
>>
>> + { ngx_string("mp4_exact_start"),
>> + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
>
> NGX_CONF_TAKE1 -> NGX_CONF_FLAG
>
>> + ngx_conf_set_flag_slot,
>> + NGX_HTTP_LOC_CONF_OFFSET,
>> + offsetof(ngx_http_mp4_conf_t, exact_start),
>> + NULL },
>> +
>> ngx_null_command
>> };
>>
>> @@ -2156,6 +2164,83 @@
>>
>>
>> static ngx_int_t
>> +ngx_http_mp4_exact_start_video(ngx_http_mp4_file_t *mp4, ngx_http_mp4_trak_t *trak)
>> +{
>> + uint32_t n, speedup_samples, current_count;
>> + ngx_uint_t sample_keyframe, start_sample_exact;
>> + ngx_mp4_stts_entry_t *entry, *entries_array;
>> + ngx_buf_t *data;
>> +
>> + data = trak->out[NGX_HTTP_MP4_STTS_DATA].buf;
>> +
>> + // Find the keyframe just before the desired start time - so that we can emit an mp4
>> + // where the first frame is a keyframe. We'll "speed up" the first frames to 1000x
>> + // normal speed (typically), so they won't be noticed. But this way, perceptively,
>> + // playback of the _video_ track can start immediately
>> + // (and not have to wait until the keyframe _after_ the desired starting time frame).
>> + start_sample_exact = trak->start_sample;
>> + for (n = 0; n < trak->sync_samples_entries; n++) {
>> + // each element of array is the sample number of a keyframe
>> + // sync samples starts from 1 -- so subtract 1
>> + sample_keyframe = ngx_mp4_get_32value(trak->stss_data_buf.pos + (n * 4)) - 1;
>
> This can be simplified by introducing entry/end variables like we usually do.
>
> Also, we don't access trak->stss_data_buf directly, but prefer
> trak->out[NGX_HTTP_MP4_STSS_ATOM].buf.
>
> ngx_http_mp4_crop_stss_data() provides an example of iterating over stss atom.
>
>> + if (sample_keyframe <= trak->start_sample) {
>> + start_sample_exact = sample_keyframe;
>> + }
>> + if (sample_keyframe >= trak->start_sample) {
>> + break;
>> + }
>> + }
>> +
>> + if (start_sample_exact < trak->start_sample) {
>> + // We're going to prepend an entry with duration=1 for the frames we want to "not see".
>> + // MOST of the time (eg: constant video framerate),
>> + // we're taking a single element entry array and making it two.
>> + speedup_samples = trak->start_sample - start_sample_exact;
>> +
>> + ngx_log_debug3(NGX_LOG_DEBUG_HTTP, mp4->file.log, 0,
>> + "exact trak start_sample move %l to %l (speed up %d samples)\n",
>> + trak->start_sample, start_sample_exact, speedup_samples);
>> +
>> + entries_array = ngx_palloc(mp4->request->pool,
>> + (1 + trak->time_to_sample_entries) * sizeof(ngx_mp4_stts_entry_t));
>> + if (entries_array == NULL) {
>> + return NGX_ERROR;
>> + }
>> + entry = &(entries_array[1]);
>> + ngx_memcpy(entry, (ngx_mp4_stts_entry_t *)data->pos,
>> + trak->time_to_sample_entries * sizeof(ngx_mp4_stts_entry_t));
>
> This reallocation can be avoided. Look at NGX_HTTP_MP4_STSC_START buffer
> as an example of that. A new 1-element optional buffer NGX_HTTP_MP4_STTS_START
> can be introduced right before the stts atom data.
>
>> + current_count = ngx_mp4_get_32value(entry->count);
>> + ngx_log_debug1(NGX_LOG_DEBUG_HTTP, mp4->file.log, 0,
>> + "exact split in 2 video STTS entry from count:%d", current_count);
>> +
>> + if (current_count <= speedup_samples) {
>> + return NGX_ERROR;
>> + }
>> +
>> + ngx_mp4_set_32value(entry->count, current_count - speedup_samples);
>> + ngx_log_debug2(NGX_LOG_DEBUG_HTTP, mp4->file.log, 0,
>> + "exact split new[1]: count:%d duration:%d",
>> + ngx_mp4_get_32value(entry->count),
>> + ngx_mp4_get_32value(entry->duration));
>> + entry--;
>> + ngx_mp4_set_32value(entry->count, speedup_samples);
>> + ngx_mp4_set_32value(entry->duration, 1);
>> + ngx_log_debug1(NGX_LOG_DEBUG_HTTP, mp4->file.log, 0,
>> + "exact split new[0]: count:%d duration:1",
>> + ngx_mp4_get_32value(entry->count));
>> +
>> + data->pos = (u_char *) entry;
>> + trak->time_to_sample_entries++;
>> + trak->start_sample = start_sample_exact;
>> + data->last = (u_char *) (entry + trak->time_to_sample_entries);
>> + }
>> +
>> + return NGX_OK;
>> +}
>> +
>> +
>> +static ngx_int_t
>> ngx_http_mp4_crop_stts_data(ngx_http_mp4_file_t *mp4,
>> ngx_http_mp4_trak_t *trak, ngx_uint_t start)
>> {
>> @@ -2164,6 +2249,8 @@
>> ngx_buf_t *data;
>> ngx_uint_t start_sample, entries, start_sec;
>> ngx_mp4_stts_entry_t *entry, *end;
>> + ngx_http_mp4_conf_t *conf;
>> +
>
> No need for a new empty line here.
>
>> if (start) {
>> start_sec = mp4->start;
>> @@ -2238,6 +2325,10 @@
>> "start_sample:%ui, new count:%uD",
>> trak->start_sample, count - rest);
>>
>> + conf = ngx_http_get_module_loc_conf(mp4->request, ngx_http_mp4_module);
>> + if (conf->exact_start) {
>> + ngx_http_mp4_exact_start_video(mp4, trak);
>> + }
>> } else {
>> ngx_mp4_set_32value(entry->count, rest);
>> data->last = (u_char *) (entry + 1);
>> @@ -3590,6 +3681,7 @@
>>
>> conf->buffer_size = NGX_CONF_UNSET_SIZE;
>> conf->max_buffer_size = NGX_CONF_UNSET_SIZE;
>> + conf->exact_start = NGX_CONF_UNSET;
>
> This is not enough, a merge is needed too.
>
>>
>> return conf;
>> }
>> _______________________________________________
>> nginx-devel mailing list
>> nginx-devel@nginx.org
>> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
> I've made a POC patch which incorporates the issues I've mentioned.
> I didn't test is properly and the directive name is still not perfect.
>
> --
> Roman Arutyunyan
> <mp4-exact-arut.txt>_______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel

-Tracey
@tracey_pooh
TV Architect https://archive.org/tv https://archive.org/tv





_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel