[FFmpeg-devel] [PATCH] Psygnosis YOP demuxer

[Michael Niedermayer]

On Tue, Aug 11, 2009 at 12:03:41AM -0400, Thomas Higdon wrote:
> Thanks for taking a look. Responses below.
[...]
> >> + ? ?av_set_pts_info(video_stream, 32, 1, frame_rate);
> >> + ? ?video_stream->nb_frames = num_frames;
> >> +
> >> + ? ?// Extra data that will be passed to the decoder
> >> + ? ?video_stream->codec->extradata_size = 5;
> >> +
> >> + ? ?video_stream->codec->extradata =
> >> + ? ? ? ?av_mallocz(video_stream->codec->extradata_size +
> >> + ? ? ? ? ? ? ? ? ? FF_INPUT_BUFFER_PADDING_SIZE);
> >> +
> >> + ? ?if (!video_stream->codec->extradata) {
> >> + ? ? ? ?return AVERROR(ENOMEM);
> >> + ? ?}
> >> +
> >
> >> + ? ?AV_WL8(video_stream->codec->extradata, yop->num_pal_colors);
> >> + ? ?AV_WL8(video_stream->codec->extradata + 1, firstcolor_odd);
> >> + ? ?AV_WL8(video_stream->codec->extradata + 2, firstcolor_even);
> >> + ? ?AV_WL16(video_stream->codec->extradata + 3, sound_chunk_length);
> >
> > this should be read with a single get_buffer() into extradata id guess
> 
> I'm not sure how this is more advantageous. I need num_pal_colors
> later in yop_read_packet(), so I can't just spirit that away into
> extradata using get_buffer(). sound_chunk_length is also discontinuous
> in the input buffer from the rest of the variables in extradata, so it
> can't be read in one call to get_buffer. Also, to me it just seems
> clearer to explicitly enumerate the variables going into extradata,
> especially since the decoder writer will later have to figure out how
> to pull them out.

the format of the global header in extradata is not supposed to be decided
by the implementation
2 demuxers supporting the same codec need to use the same extradata format
thus staying with how it happens to be stored increases the chances that
it will work.
and about the discontinuity, the bytes between are unused and skiped not
used by the demxuer, their ommision from extradata looks wrong, though
maybe its right, one would have to know what the are first


[...]
> >> + ? ?if (yop->stream == 0) { // audio
> >> + ? ? ? ?st = s->streams[yop->stream];
> >> +
> >> + ? ? ? ?// skip over video data
> >> + ? ? ? ?url_fskip(pb, skip);
> >> + ? ? ? ?ret = av_get_packet(pb, pkt, yop->sound_data_length);
> >> + ? ? ? ?if (ret < yop->sound_data_length) {
> >> + ? ? ? ? ? ?return AVERROR_EOF;
> >> + ? ? ? ?}
> >> + ? ? ? ?// seek back to beginning of packet for the video data
> >> + ? ? ? ?url_fseek(pb, -(skip + yop->sound_data_length), SEEK_CUR);
> >
> > can seek back be avoided?
> > because seeking back can cause problems if the underlaying protocol doesnt
> > support seeking back
> 
> By underlying protocol, I assume that you mean if the data is coming
> from a network stream or something of that nature. The problem, as I
> see it, is that each packet is composed like this:
> 
> 1. <palette>
> 2. <audio_data>
> 3. <video_data>
> 
> The video decoder needs parts 1 and 3, while the audio decoder needs
> 2. I'm really at a loss as to how to avoid the seek back. Would it be
> valid to call av_get_packet() once for the palette and again for the
> video data on a subsequent call to yop_read_packet()? Would
> decode_frame then have to return if it didn't have enough data yet,
> and only decode the frame when it had both the palette and the video
> data? It's not completely clear to me if/how this would work.

id create a packet of size palette + video and one of size audio and
just fill them with the data sequentially then return one and in the
next call the other

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090811/b02ecc83/attachment.pgp>