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 tra...@archive.org <mailto:tra...@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 <a...@nginx.com> wrote: > > Hi Tracey, > > On Tue, Jun 15, 2021 at 03:49:48PM -0700, Tracey Jaquith wrote: >> # HG changeset patch >> # User Tracey Jaquith <tra...@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
