[MPlayer-dev-eng] [PATCH] Movie fragments support for demux_mov
David Romacho
romaxin2002 at yahoo.com.au
Sun Jan 13 11:12:32 CET 2008
Reimar Döffinger wrote:
> 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.
>
I removed this from the patch. I will send a separate patch for this.
>> @@ -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).
>
Removed from patch.
>> @@ -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.
>
>
Yes, I 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.
>
Comment removed.
>> @@ -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.
>
Fixed.
>> @@ -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.
>
Removed from patch.
>> @@ -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.
>
Fixed.
>> + //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.
>
I would prefer remove all the unnecessary messages in the final version
of the patch.
>> @@ -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.
>
Fixed.
>> + } 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...
>
Fixed (I think).
>> + // 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.
>
Added overflow and NULL check. On error, the function just returns.
Would a warning message be necessary here?
>
>> + // 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.
>
Fixed.
>> + if( trak->keyframes_size>0 && trak->keyframes_size>0 ) {
>>
>
> The same condition twice?
>
Fixed.
>> + 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().
>
Fixed.
>> + 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.
>
I am not using a separate function for those checks. There are many
calls to realloc_struct in the same demux_mov.c file so this NULL check
could go into that function. BTW, the NULL check is not done in any part
of demux_mov.c
>> + // 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.
>
mmm...maybe the common function will look nasty with this check being toggleable ...I will create this function if you still consider it necessary.
>> + 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.
>
>
Fixed.
>> +// Helper fnction, should be moved to stream.h
>>
>
> "fUnction", also this is too mov-specific to belong into stream.h
>
Orthography fixed and comment about moving to stream.h removed.
>> + 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();
>
Using this in all the version and flags read.
>> + 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.
>
Fixed.
>> + 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.
>
Yes it is clearer. Using it.
Changed to int.
I am using the same variable names as they appear in the IEC/ITU
14496-12 documentation. In this way it is easier to understand the code
(if you use the documentation).
>> + 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.
>
>
Same thing here, I am using variable names taken from standard
documentation. If you still don't like those names I will change them.
>> + 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.
>
Fixed.
>> + // First read: parse tfhd and count number of truns
>> + while((pos+len)<endpos){
>>
>
> Useless () around pos+len.
>
Fixed.
>> + 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?
>
Actually I separate function would increase the size of the patch.
Making the declaration and initialization in one line is shorter, I
think. Again, I will create this function if you still think it is
necessary.
>> + // 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.
>
Added defines with the meaning of those constants. I took the name from
documentation.
>> + // 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.
>
Fixed.
> 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.
>
>
I could merge lschunks_inmoof and lschunks_inmfra in one function.
lschunks_intraf and lschunks_inmvex could be merged also.
Would that be ok?
> Greetings,
> Reimar Döffinger
> _______________________________________________
> MPlayer-dev-eng mailing list
> MPlayer-dev-eng at mplayerhq.hu
> http://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
>
>
Greetings,
David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: movie_fragments_v3.patch
Type: text/x-patch
Size: 30257 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20080113/7a5e0a80/attachment.bin>
More information about the MPlayer-dev-eng
mailing list