[MPlayer-dev-eng] [PATCH] Movie fragments support for demux_mov
Reimar Döffinger
Reimar.Doeffinger at stud.uni-karlsruhe.de
Thu Dec 20 16:36:11 CET 2007
Hello,
On Thu, Dec 20, 2007 at 02:20:17PM +0100, David Romacho wrote:
[...]
> @@ -226,9 +255,8 @@
> }
>
> if (trak->samples_size < s) {
> - mp_msg(MSGT_DEMUX, MSGL_WARN,
> - "MOV: durmap or chunkmap bigger than sample count (%i vs %i)\n",
> - s, trak->samples_size);
> + if( trak->samples_size )
> + mp_msg(MSGT_DEMUX, MSGL_WARN,"MOV: durmap or chunkmap bigger than sample count (%i vs %i)\n", s, trak->samples_size);
This seems to make sense to me independent of the rest, give me a commit
message indicating why it makes sense and I'll commit it right away.
> @@ -272,9 +300,21 @@
> el->frames=0; continue;
> }
> // find start sample
> + // If track has keyframes, choose the nearest
> + // (earlier) keyframe. Otherwise, just choose the nearest sample.
> + if(trak->keyframes_size) {
> + unsigned int kfi;
> + for(kfi=0;kfi<trak->keyframes_size;kfi++){
> + if(trak->samples[trak->keyframes[kfi+1]].pts > pts){
> + sample = trak->keyframes[kfi];
> + break;
> + }
> + }
> + } else {
> for(;sample<trak->samples_size;sample++){
> if(pts<=trak->samples[sample].pts) break;
> }
> + }
Same here, this really should be applied separately because some people
might not like this change at all (and ideally it would choose the
previous or next keyframe depending on flags in some future version).
> @@ -295,12 +335,24 @@
>
> #define MOV_MAX_TRACKS 256
> #define MOV_MAX_SUBLEN 1024
> +#define MOV_MAX_FRAGMENTS 256
> +#define MOV_MAX_TRAFS 256
>
> typedef struct {
> off_t moov_start;
> off_t moov_end;
> - off_t mdat_start;
> - off_t mdat_end;
> + off_t mdat_start[MOV_MAX_FRAGMENTS];
> + off_t mdat_end[MOV_MAX_FRAGMENTS];
> + off_t moof_start[MOV_MAX_FRAGMENTS];
> + off_t moof_end[MOV_MAX_FRAGMENTS];
> + off_t mfra_start;
> + off_t mfra_end;
> + int mvex_counter;
> + mov_traf_t mvexs[MOV_MAX_TRACKS];
> + int mdat_counter;
> + int moof_counter;
> + int traf_counter;
> + mov_traf_t* trafs[MOV_MAX_TRAFS];
> int track_db;
> mov_track_t* tracks[MOV_MAX_TRACKS];
It fits with the rest of the code so I won't request you to change it,
but all these static arrays are quite a waste of memory.
> @@ -498,6 +553,20 @@
> case MOV_FOURCC('P','I','C','T'):
> /* dunno what, but we shoudl ignore it */
> break;
> + case MOV_FOURCC('m','f','r','a'): {
> + off_t pos = stream_tell(demuxer->stream);
> + priv->mfra_start = pos;
> + priv->mfra_end = pos+len-skipped;
> + mp_msg(MSGT_DEMUX,MSGL_V,"MOV: Movie Fragment Random Access Box 'mfra' (%d bytes)\n",(int)len);
> + } break;
> + /* Now we handle moofs */
> + case MOV_FOURCC('m','o','o','f'):{
That IMO is a really pointless comment, actually even confusing, I first
thought it to mean that there would be a fall-through between those
cases.
> @@ -546,6 +615,15 @@
> free(track);
> }
> }
> + for (i = 0; i < MOV_MAX_TRAFS; i++) {
> + mov_traf_t *traf = priv->trafs[i];
> + if (traf) {
> + //free all traf fileds here
What are "fileds"? Also you use tabs in the block, but the surrounding
code does not seem to use tabs, so better use spaces-only as well.
> @@ -1106,8 +1184,31 @@
> // printf("pos=%d max=%d\n",pos,trak->stdata_len);
> }
> }
> - sh->fps=trak->timescale/
> - ((trak->durmap_size>=1)?(float)trak->durmap[0].dur:1);
> +
> + if(trak->durmap_size>=1){
> + int i,totsmpls=0;
> + double mean=0.0;
> + //mp_msg(MSGT_DEMUX, MSGL_DBG2,"timescale=%d durmap_siz=%d\ndurmap=\n",
> + // trak->timescale,trak->durmap_size);
> + for(i=0;i<trak->durmap_size;totsmpls+=trak->durmap[i++].num) {
> + mean+=trak->durmap[i].num*trak->durmap[i].dur;
> + //mp_msg(MSGT_DEMUX,MSGL_DBG2,"{%d,%d},", trak->durmap[i].num,
> + // trak->durmap[i].dur);
> + }
> + mean/=totsmpls;
> + //mp_msg(MSGT_DEMUX,MSGL_DBG2,"\n");
> + /*be conservative w/this adjustment for the time being. decreasing
> + the framerate and encoding w/o softskip is bound to cause decode
> + errors and possibly cause the output framerate to be suboptimal.
> + really this should be marked as variable framerate in the case
> + of durmap_size>1
> + */
> + if(mean>(double)trak->durmap[0].dur) mean= trak->durmap[0].dur;
> + sh->fps=trak->timescale/mean;
> + }else{
> + sh->fps=trak->timescale;
> + }
IMO this is no good, sh->fps only makes sense for fixed-fps files, and
such a hack only complicates the code but can't really save it.
As long as you return correct timestamps for the packets, -correct-pts
should fix it and is the way to go.
> @@ -1283,6 +1384,12 @@
> return;
> } else { /* not in track */
> switch(id) {
> + case MOV_FOURCC('m','v','e','x'): {
> + mp_msg(MSGT_DEMUX, MSGL_V,"MOV:Movie Extends Box (%d bytes)\n",(int)len);
> + lschunks_inmvex(demuxer, 0, pos+len, NULL);
> + break;
> + }
> +
> case MOV_FOURCC('m','v','h','d'): {
> int version = stream_read_char(demuxer->stream);
> stream_skip(demuxer->stream, (version == 1) ? 19 : 11);
Please make the indentation match the following code.
> + //mp_msg(MSGT_DEMUX,MSGL_V,"\nParsing mfra\n");
This applies to other cases as well: Either you expect those debugging
messages to be still useful in the future, then leave them in without
commenting out, or remove them completely.
If you still intend to do some debugging you can of course leave them in
till the final version of the patch, but then it should be cleaned up.
> @@ -2301,10 +2446,23 @@
> case DEMUXER_CTRL_GET_PERCENT_POS:
> {
> off_t pos = track->pos;
> - if (track->durmap_size >= 1)
> - pos *= track->durmap[0].dur;
> - *((int *)arg) = (int)(100 * pos / track->length);
> - return DEMUXER_CTRL_OK;
> + int dur_count = 0;
> + int sample_count = 0;
> + int i;
> + if (track->durmap_size == 1)
> + pos *= track->durmap[0].dur;
> + else for( i=0; i<track->durmap_size; i++ ) {
> + if( pos>sample_count+track->durmap[i].num ) {
> + dur_count += track->durmap[i].num*track->durmap[i].dur;
> + sample_count += track->durmap[i].num;
> + } else {
> + dur_count += (((int)pos)-sample_count)*track->durmap[i].dur;
> + pos = dur_count;
> + break;
> + }
> + }
> + *((int *)arg) = (int)(100 *pos / track->length);
> + return DEMUXER_CTRL_OK;
Use spaces like the original code. That will also reduce the size of the
patch by two lines at least.
> + } else {
> + trak->samples = realloc_struct(trak->samples, trak->samples_size+traf->samples_size, sizeof(mov_sample_t));
> + for(i=0;i<traf->samples_size;i++)
> + trak->samples[trak->samples_size + i].size = traf->samples[i].size;
> +
> + trak->samples_size += traf->samples_size;
> + //mp_msg(MSGT_DEMUX, MSGL_V, "Added %d samples, total %d\n",traf->samples_size,trak->samples_size);
> + }
In this and quite a few other places your indentation is messed up or at
least inconsistent.
I'd prefer to not have that in new code...
> + // Add chunks
> + trak->chunks = realloc_struct(trak->chunks, trak->chunks_size+traf->chunks_size, sizeof(mov_chunk_t));
> + memcpy( &trak->chunks[trak->chunks_size], traf->chunks, traf->chunks_size*sizeof( mov_chunk_t ) );
> + trak->chunks_size += traf->chunks_size;
Hm.. I think this code may need an integer overflow check for the
trak->chunks_size+traf->chunks_size and possibly a != NULL check for the
trak->chunks after realloc to be safe...
Possibly there is no problem since the values are limited in size so no
overflow can happen, in this case a comment would probably be good.
> + // Add durmaps
> + if( !constant_sample_size ) {
> + trak->durmap = realloc_struct(trak->durmap, trak->durmap_size+traf->durmap_size, sizeof(mov_durmap_t));
> + memcpy( &trak->durmap[trak->durmap_size], traf->durmap, traf->durmap_size*sizeof(mov_durmap_t) );
> + /*for(i=0;i<traf->durmap_size;i++){
> + trak->durmap[trak->durmap_size + i].dur = traf->durmap[i].dur;
> + trak->durmap[trak->durmap_size + i].num = traf->durmap[i].num;
> + }*/
> + trak->durmap_size += traf->durmap_size;
Same problem here.
And memcpy is weirdly indented.
> + if( trak->keyframes_size>0 && trak->keyframes_size>0 ) {
The same condition twice?
> + trak->keyframes = realloc(trak->keyframes, (trak->keyframes_size+traf->keyframes_size)*sizeof(unsigned int));
> + for( i=0; i<traf->keyframes_size; i++ ) {
> + trak->keyframes[trak->keyframes_size+i]=original_samples_size+trak->keyframes[i];
> + }
> + trak->keyframes_size += traf->keyframes_size;
> + } else if( trak->keyframes_size>0 ) { // no mfra supplied but needed, need to create it??
> + trak->keyframes = realloc(trak->keyframes, (trak->keyframes_size+traf->samples_size)*sizeof(unsigned int));
> + for( i=0; i<traf->samples_size; i++ ) {
> + trak->keyframes[trak->keyframes_size+i]=original_samples_size+i;
> + }
> + trak->keyframes_size += traf->samples_size;
Same problem with missing checks again, but even worse since you did not
even use realloc_struct to avoid the problems due to multiplying with
sizeof().
> + if (trak->samples_size < s) {
> + if( trak->samples_size ) {
> + mp_msg(MSGT_DEMUX, MSGL_WARN,"MOV: durmap or chunkmap bigger than sample count (%i vs %i)\n",s, trak->samples_size);
> + }
> + trak->samples = realloc_struct(trak->samples, s, sizeof(mov_sample_t));
> + for(i=0;i<s;i++)
> + trak->samples[trak->samples_size+i].size = trak->samplesize;
> + trak->samples_size = s;
Once more the problem. Might be worth to make an extra function to do
the realloc + checks.
Note that the reason why these _need_ the !=NULL checks in difference to
the other similar places in this file (where the lack of it is mostly
only "bad style") is that they do not start with accessing at index 0,
making a program write to e.g. ((int*)NULL)[0] is only a DoS, whereas
a write to ((int*)NULL)[x] with arbitrary x is easily exploitable.
> + // calc pts:
> + s=0;
> + for(j=0;j<trak->durmap_size;j++){
> + for(i=0;i<trak->durmap[j].num;i++){
> + trak->samples[s].pts=pts;
> + ++s;
> + pts+=trak->durmap[j].dur;
> + if(!trak->durmap[j].dur)
> + pts+=trak->durmap[0].dur;
> + }
> + }
There is already almost the same code but without the
"if(!trak->durmap[j].dur)" check in this file, this should maybe be
factored out into a common function.
> + if( trak->length != pts ) {
> + // normal behavior when TRAFS are present
> + //mp_msg(MSGT_DEMUX, MSGL_WARN,
> + // "MOV: track length is: %d, and durmap length is: %d\n", trak->length, pts );
> + trak->length = pts;
> + }
The "if" is quite pointless, at least if there's only the assignment in
it.
> +// Helper fnction, should be moved to stream.h
"fUnction", also this is too mov-specific to belong into stream.h
> + id=stream_read_dword(demuxer->stream);
> + switch(id) {
> + case MOV_FOURCC('t','f','r','a'): {
> + int version = stream_read_dword(demuxer->stream);
> + int flags = version & 0x0ffffff;
> + version = version >> 24;
version = stream_read_char();
flags = stream_read_int24();
> + mp_msg(MSGT_DEMUX, MSGL_V,"MOV:\tTrack Fragment Random Access Box 'tfra' (%d bytes): v=%d flags=%d\n",(int)len, (int)version,(int)flags);
> + uint32_t track_ID = stream_read_dword(demuxer->stream);
Please do not mix declarations and code.
> + uint32_t tmp = stream_read_dword(demuxer->stream);
> + uint32_t reserved = tmp >> 6;
> + uint32_t length_size_of_traf_num = (tmp & 0x030) >> 4;
> + uint32_t length_size_of_trun_num = (tmp & 0x0c) >> 2;
> + uint32_t length_size_of_sample_num = (tmp & 0x03);
(tmp >> 4) & 3;
(tmp >> 2) & 3;
tmp & 3;
is clearer IMO.
Also you shouldn't use uint32_t without a reason, "int" works just well
here.
A shorter variable name would be clearer as well IMO, something like traf_num_bytes maybe.
> + uint32_t number_of_entry = stream_read_dword(demuxer->stream);
> + mp_msg(MSGT_DEMUX, MSGL_DBG3,"MOV:\t\ttrack_ID = %d\n",track_ID);
> + mp_msg(MSGT_DEMUX, MSGL_DBG3,"MOV:\t\treserved = %d\n",reserved);
> + mp_msg(MSGT_DEMUX, MSGL_DBG3,"MOV:\t\tlength_size_of_traf_num = %d\n",length_size_of_traf_num);
> + mp_msg(MSGT_DEMUX, MSGL_DBG3,"MOV:\t\tlength_size_of_trun_num = %d\n",length_size_of_trun_num);
> + mp_msg(MSGT_DEMUX, MSGL_DBG3,"MOV:\t\tlength_size_of_sample_num = %d\n",length_size_of_sample_num);
> + mp_msg(MSGT_DEMUX, MSGL_DBG3,"MOV:\t\tnumber_of_entry = %d\n",number_of_entry);
> + for(i=0;i<number_of_entry;i++){
Hmm. I think that variable should be named "number_of_entries".
Though num_entries would be enough for me personally.
> + case MOV_FOURCC('m','f','r','o'):{
> + int version = stream_read_dword(demuxer->stream);
> + int flags = version & 0x0ffffff;
> + version = version >> 24;
> + mp_msg(MSGT_DEMUX, MSGL_V,"MOV:\tMovie Fragment Random Access Offset Box 'mfro' (%d bytes): v=%d flags=%d\n",(int)len, (int)version,(int)flags);
Same comment as about the similar block above.
> + // First read: parse tfhd and count number of truns
> + while((pos+len)<endpos){
Useless () around pos+len.
> + case MOV_FOURCC('t','f','h','d'): {
> + int k, version = stream_read_dword(demuxer->stream);
> + int tf_flags = version & 0x0ffffff;
> + version = version >> 24;
As frequent as it is, how about a function that reads flags and version?
> + // now continue parse
> + if(tf_flags & 0x000001) {
> + traf->base_data_offset = stream_read_qword(demuxer->stream);
> + mp_msg(MSGT_DEMUX, MSGL_DBG3,"MOV:\t\tbase_data_offset=%d\n",traf->base_data_offset);
> + }
> + if(tf_flags & 0x000002) {
> + traf->sample_description_index = stream_read_dword(demuxer->stream);
> + mp_msg(MSGT_DEMUX, MSGL_DBG3,"MOV:\t\tsample_description_index=%d\n",traf->sample_description_index);
> + }
> + if(tf_flags & 0x000008) {
> + traf->default_sample_duration = stream_read_dword(demuxer->stream);
> + mp_msg(MSGT_DEMUX, MSGL_DBG3,"MOV:\t\tdefault_sample_duration=%d\n",traf->default_sample_duration);
> + }
> + if(tf_flags & 0x000010) {
> + traf->default_sample_size = stream_read_dword(demuxer->stream);
> + mp_msg(MSGT_DEMUX, MSGL_DBG3,"MOV:\t\tdefault_sample_size=%d\n",traf->default_sample_size);
> + }
> + if(tf_flags & 0x000020) {
> + traf->default_sample_flags = stream_read_dword(demuxer->stream);
> + mp_msg(MSGT_DEMUX, MSGL_DBG3,"MOV:\t\tdefault_sample_flags=%d\n",traf->default_sample_flags);
> + }
Defines for those flag constants might look nice, esp. if you know
proper names for them.
> + // Alloc arrays
> + if( found_variable_samplesize ) {
> + traf->samples = (mov_sample_t*) calloc(traf->samples_size,sizeof(mov_sample_t));
> + } else {
> + traf->samples_size=0;
> + }
> + traf->durmap = (mov_durmap_t*)calloc(traf->durmap_size,sizeof(mov_durmap_t));
> + traf->chunks = (mov_chunk_t*)calloc(traf->chunks_size,sizeof(mov_chunk_t));
useless casts.
I also wonder if it wouldn't be possible to avoid some of the code
duplication in all those different parsing functions, like reading
length, checking it, skipping unknown parts etc.
Greetings,
Reimar Döffinger
More information about the MPlayer-dev-eng
mailing list