[Ffmpeg-devel] Re: [PATCH] Tiertex .SEQ files support

[Michael Niedermayer]

Hi

On Mon, Sep 25, 2006 at 01:44:39PM +0200, Gregory Montoir wrote:
[...]
> >[...]
> >>>>+        seq_fill_buffer(seq, seq->current_frame_data[5],
> >>>>+          seq->current_frame_data + offset,
> >>>>+          end_offset - offset);
> >>>>+    }
> >>>>+    offset = LE_16(&seq->current_frame_data[10]);
> >>>>+    if (offset != 0) {
> >>>>+        end_offset = LE_16(&seq->current_frame_data[12]);
> >>>>+        if (end_offset == 0)
> >>>>+            end_offset = LE_16(&seq->current_frame_data[14]);
> >>>>+        seq_fill_buffer(seq, seq->current_frame_data[6],
> >>>>+          seq->current_frame_data + offset,
> >>>>+          end_offset - offset);
> >>>>+    }
> >>>>+    offset = LE_16(&seq->current_frame_data[12]);
> >>>>+    if (offset != 0) {
> >>>>+        end_offset = LE_16(&seq->current_frame_data[14]);
> >>>>+        seq_fill_buffer(seq, seq->current_frame_data[7],
> >>>>+          seq->current_frame_data + offset,
> >>>>+          end_offset - offset);
> >>>>+    }
> >>>for(i=0; i<3; i++)
> >>>   buf_num[i]= get_byte();
> >>>for(i=0; i<4; i++)
> >>>   offset[i]= get_le16();
> >>>end_offset[3]= 0;
> >>>for(i=2; i>=0; i--)
> >>>   end_offset[i]= offset[i+1] ? offset[i+1] : end_offset[i+1]
> >>>
> >>>for(i=0; i<3; i++){
> >>>   if(offset[i])
> >>>       seq_fill_buffer(seq, buf_num[i],
> >>>           seq->current_frame_data + offset[i],
> >>>           end_offset[i] - offset[i]);
> >>>}
> >>>
> >>>dont you think that something like the above code is more readable? also 
> >>>there are no url_fseek() needed ...
> >>Indeed, the for loop makes the code more friendly.
> >>
> >>But still, I don't see how you can use the get_* functions on
> >>ByteIOContext *and not* using url_fseek later on... Looking at your
> >>pseudo code, seq_fill_buffer takes an offset as argument. So if you want
> >>to copy data from a given offset, without having read the whole 6kb
> >>buffer before, I will have to "seek". Same goes for the sound and
> >>palette data.
> >>
> >>Am I missing something ?
> >
> >the few url_fseek() will likely be faster then the memcpy(), benchmarks
> >are of course welcome ...
> 
> Well, there is no real need to do benchmarks. If you don't read the
> whole 6kb frame buffer at once, you'll need to do at most 6 reads by
> frame (1 for palette data, 1 for audio and 4 for video) and do the
> appropriate seeking before each read. Thus, number of I/O will be much
> more important.

libavformat uses an internal buffer (see aviobuf.c) so there should be no
additional external I/O

[...]

patch looks ok

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is