[MPlayer-dev-eng] [PATCH] Movie fragments support for demux_mov
David Romacho
romaxin2002 at yahoo.com.au
Thu Dec 20 14:20:17 CET 2007
See comments below
Ulion wrote:
> 2007/12/10, David Romacho <romaxin2002 at yahoo.com.au>:
>
>> David Romacho wrote:
>>
>>> Hi all,
>>>
>>> The goal of these patches is to provide MPlayer with the movie
>>> fragment feature present in mov/mp4 container (ISO/IEC 14496-12
>>> standard). This feature is used by some digital cameras that save the
>>> video in several fragments. Currently MPlayer just recognises the
>>> first movie fragment and does not play the rest of fragments.
>>>
>>> I split the previous patch into 3 different patches. They are supposed
>>> to be applied one right after the other against demux_mov.c
>>> MPlayer should compile after applying each patch but support for movie
>>> fragments won't be fully operational till last patch is applied.
>>> So, once all patches are applied, MPlayer should playback the movie
>>> fragment test samples (http://samples.mplayerhq.hu/mov/fragments/)
>>> completely. The head of MPlayer just plays the first 15 seconds of
>>> them (the first movie fragment).
>>>
>>> ...................
>>>
>> No one is interested in this feature ? The code is fully functional so
>> after applying the patch mplayer is able to reproduce correctly a
>> certain kind of MP4/MOV files. No one is encouraged to review these
>> patches ?
>>
>
> I can try, review:
>
> + // 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-1;kfi++){
> + if(trak->samples[trak->keyframes[kfi+1]].pts > pts){
> + sample = trak->keyframes[kfi];
> + break;
> + }
> + }
>
> Last keyframe is ignored, maybe you should loop from back to front.
>
Ooops, you're right there! fixed
> + mp_msg(MSGT_DEMUX,MSGL_V,"MOV: Movie DATA #%"PRIx64"
> found!\n",(int64_t)priv->mdat_counter);
> + priv->mdat_start[priv->mdat_counter]=stream_tell(demuxer->stream);
> + priv->mdat_end[priv->mdat_counter]=priv->mdat_start[priv->mdat_counter]+len-skipped;
> + priv->mdat_counter++;
> + mp_msg(MSGT_DEMUX,MSGL_DBG2,"MOV: Movie data #%"PRIx64": start:
> %"PRIx64" end: %"PRIx64"\n",
> + (int64_t)priv->mdat_counter,(int64_t)priv->mdat_start[priv->mdat_counter-1],
> + (int64_t)priv->mdat_end[priv->mdat_counter-1]);
>
> mdat_counter is not off_t or int64, need not PRIx64 to print it.
>
> And should keep the indent of the original code (N tabs prefix + M
> spaces, 0<=M<7)
> And for better alignment with any tab size, the wrapped line should
> use the previous line's prefix tab/spaces with additional spaces to
> align the two lines to the first parameter of the function call.
> But anyway the indent style of the original code is really not good,
> it force people use tab size 8 else may got a bad view.
>
>
>
done
> + //if(flags==3){
> // if we're over the headers, then we can stop parsing here!
> - demuxer->priv=priv;
> - return DEMUXER_TYPE_MOV;
> - }
> + //demuxer->priv=priv;
> + //return DEMUXER_TYPE_MOV;
> + //}
>
> use /* */ to comment here will looks better.
>
>
done
> + for (i = 0; i < MOV_MAX_TRAFS; i++) {
> + mov_traf_t *traf = priv->trafs[i];
> + if (traf) {
> + // free all traf fileds here
> + free(traf->samples);
> + free(traf->durmap);
> + free(traf->chunks);
> + }
> + }
>
> ident size should be fixed, and comment not ident correctly.
>
>
>
done
> + /*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
> + */
>
> keep cols within 80.
>
>
done
> + if( (trak->samplesize == traf->default_sample_size) ||
> traf->default_sample_size==0 ){
> + constant_sample_size = 1;
> + } else {
> + s=0;
> + for(i=0;i<trak->chunks_size;i++){
> + trak->chunks[i].sample=s;
> + s+=trak->chunks[i].size;
> + }
>
> indent fix
>
>
done
> + } 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);
> + }
>
> same as above.
>
>
done
> + // Add chunks
> + trak->chunks = realloc_struct(trak->chunks,
> trak->chunks_size+traf->chunks_size, sizeof(mov_chunk_t));
> + for(i=0;i<traf->chunks_size;i++){
> + trak->chunks[trak->chunks_size + i].size = traf->chunks[i].size;
> + trak->chunks[trak->chunks_size + i].pos = traf->chunks[i].pos;
> + trak->chunks[trak->chunks_size + i].desc = traf->chunks[i].desc;
> + }
>
> Does the chunk struct member sample need be assigned also or just
> leave it in undefined value.
> Or I suggested use one memcpy to replace the loop.
>
>
memcpy used
> + // Add durmaps
> + if( !constant_sample_size ) {
> + trak->durmap = realloc_struct(trak->durmap,
> trak->durmap_size+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;
>
> memcpy.
>
>
memcpy used
> + 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_size = s;
> + trak->samples = realloc_struct(trak->samples, s, sizeof(mov_sample_t));
> + }
>
> need to set any default value for new allocated part samples?
> samples[i].size seem will be used later without assignment.
>
>
well, the only assignment needed is for the .size member...
the function rebuild_index is a copy of the function build_index, where
those samples were equally unassigned, so they both share the same
error... but...
that line of code is only reached when the chunkmap and durmap tables in
the media file differ, so the realloc should never happen.
The only workaround I found is to fill the size with the fixed size
given in the headers as default sample size
Doing that or doing nothing, the chunk and sample search can anyway
result in an invalid file offset, so the sample wouldn't be found.
> + // 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;
> + }
> + }
>
> Is it safe to add trak->durmap[0].dur?
>
not safe, but it's the only solution we can find at this point,
otherwise we should... quit?
> + 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;
> + }
>
> indent fix.
>
>
done
> + 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);
> + int i, j, k, count_trafs;
> + mov_traf_t *traf = NULL;
> + uint64_t count_sample;
>
> declaration not in beginning of scope.
>
>
done
> + }else{
> + time = stream_read_dword(demuxer->stream);;
> + moof_offset = stream_read_dword(demuxer->stream);;
> + }
>
> ;;
>
> + // look for the specified traf
> + if( traf!= NULL ) {
> + if( traf->track_ID == track_ID ) {
> + if( count_trafs == traf_number ) { // already
> selected traf, no need to search it
> + if( trun_number<traf->chunks_size &&
> trun_number>0 ) {
>
> merge ifs
>
>
done
> + // Alloc arrays
> + if( found_variable_samplesize ) {
> + traf->samples = (mov_sample_t*)
> malloc(traf->samples_size*sizeof(mov_sample_t));
> + memset( traf->samples, 0,
> traf->samples_size*sizeof(mov_sample_t));
> + } else {
> + traf->samples_size=0;
> + }
> + traf->durmap =
> (mov_durmap_t*)malloc(traf->durmap_size*sizeof(mov_durmap_t));
> + traf->chunks = (mov_chunk_t*)malloc(traf->chunks_size*sizeof(mov_chunk_t));
> + memset( traf->durmap, 0, traf->durmap_size*sizeof(mov_durmap_t) );
> + memset( traf->chunks, 0, traf->chunks_size*sizeof(mov_chunk_t));
> +
>
> calloc to shorten code.
>
>
done
> + len = 0;
> + pos = start;
> + int sample_count = 0;
> + int durmap_count = 0;
> + int chunk_count = 0;
>
> declaration should not at beginning of scope.
>
>
done
> + priv->trafs[priv->traf_counter] = (mov_traf_t*)
> malloc(sizeof(mov_traf_t));
> + memset(priv->trafs[priv->traf_counter],0,sizeof(mov_traf_t));
>
> no force type conversion, and calloc will save one line.
>
>
>
done
I put all the changes in just one patch. If you still prefer the 3
consecutive patches I will create them (it takes quite a bit for me).
Cheers,
David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: movie_fragments_v2.patch
Type: text/x-patch
Size: 34300 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20071220/fb18a7c3/attachment.bin>
More information about the MPlayer-dev-eng
mailing list