[MPlayer-dev-eng] [PATCH] Movie fragments support for demux_mov

[Reimar Döffinger]

Hello,

Some small comments:

>> 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?

It would make the functions even bigger and even harder to understand,
that's not exactly what I had in mind, I was thinking of doing it more
like libavformat/mxf.c with tables specifying the fourcc -> parsing
function mapping.
If I have time I'll see if I can come up with something.
You're of course free to come up with something, but I admit this issue
is mostly a question of fixing the already existing code as well which
is a bit much to ask from you :-)

> +void add_traf_to_track(mov_track_t* trak,int timescale,mov_traf_t* traf);
> +void mov_rebuild_index(mov_track_t* trak,int timescale);

Hmm.. why are these not static as well?

> +    // Attach trafs
> +    for(i=0;i<priv->track_db;i++){
> +      mov_track_t* trak = priv->tracks[i];
> +      mov_traf_t *traf;
> +      for(j=0;j<priv->traf_counter;j++){
> +        traf = priv->trafs[j];
> +        if((trak->track_ID) == traf->track_ID) {
> +          add_traf_to_track(trak,priv->timescale,traf);
> +        }

One of the () and the {} are not necessary...

> @@ -2303,10 +2408,21 @@
>      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;

These last 3 lines still are there after your patch, so if they come up with a
"-" in front in the patch there's something wrong.

[...]
> +        } 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;

The thing that's wrong is that the indentation of these lines is a few
levels off...
Also, what is the point of casting pos to int in the second line?

[...]
> +	    s=0;
> +	    for(i=0;i<trak->chunks_size;i++){
> +	      trak->chunks[i].sample=s;
> +	      s+=trak->chunks[i].size;
> +	    }

> +	s=0;
> +	for(j=0;j<trak->chunks_size;j++){
> +	  trak->chunks[j].sample=s;
> +	  s+=trak->chunks[j].size;
> +	}

Maybe a init_chunks_sample function?

> +	// 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;
> +	  }
> +	}

> +	// calc sample offsets
> +	s=0;
> +	for(j=0;j<trak->chunks_size;j++){
> +	  off_t pos=trak->chunks[j].pos;
> +	  mp_msg(MSGT_DEMUX, MSGL_DBG3, "Chunk %5d: sample=%8d  off=0x%08X  size=%d\n",j,trak->chunks[j].sample,trak->chunks[j].pos, trak->chunks[j].size);
> +	  for(i=0;i<trak->chunks[j].size;i++){
> +	    trak->samples[s].pos=pos;
> +	    mp_msg(MSGT_DEMUX, MSGL_DBG3, "Sample %5d: pts=%8d  off=0x%08X  size=%d\n",s,trak->samples[s].pts,(int)trak->samples[s].pos,trak->samples[s].size);
> +	    pos+=trak->samples[s].size;
> +	    ++s;
> +	  }
> +	}

I do not know how you read code, but to me these look like ideal
candidates to put into a function with an appropriate name instead of
putting a comment above them. The comment does help me to know what it
does, but it does not help to ignore the code when I try to just
understand what the surrounding code does!
If you disagree feel free to say so, but these IMO are the perfect
examples why when something needs an explaining comment it often means that
you just should have implemented it in a more self-explaining way!

> +	  for(i=0;i<traf->durmap_size;i++){
> +	    trak->durmap[0].dur += traf->durmap[i].dur;
> +	    trak->durmap[0].num += traf->durmap[i].num;
> +	  }

> +	i = 0;
> +	for (j = 0; j < trak->durmap_size; j++) i += trak->durmap[j].num;

It would not save code, but a durmap_dur_sum and durmap_num_sum function IMHO
would make this code easier to read.

> +	        if((traf->track_ID==track_ID) && (count_trafs==traf_number) &&
> +	          (trun_number<traf->chunks_size) && (trun_number>0) ) {

The inner () are not necessary.

> +	          for( k=0, count_sample=0; k<trun_number; k++ ) count_sample += traf->chunks[k].size;
> +	          count_sample += sample_number-1;
> +	          traf->keyframes[traf->keyframes_size++] = count_sample;

> +	              for( k=0, count_sample=0; k<trun_number; k++ ) count_sample += traf->chunks[k].size;
> +	              count_sample += sample_number-1;
> +	              traf->keyframes[traf->keyframes_size++] = count_sample;

Putting this in an extra function would not save much, but it least it
would be clearer what exactly this does (e.g. does this calculate
count_sample for later use in the code? Or does this only set
traf->keyframes? or both?).

Greetings,
Reimar Döffinger