[FFmpeg-devel] [PATCH] Psygnosis YOP demuxer

Tue Aug 11 06:03:41 CEST 2009

Thanks for taking a look. Responses below.

On Mon, Aug 10, 2009 at 7:52 AM, Michael Niedermayer<michaelni at gmx.at> wrote:
> On Sun, Aug 09, 2009 at 08:57:16PM -0400, Thomas Higdon wrote:
>> On Sun, Aug 9, 2009 at 8:47 PM, Michael Niedermayer<michaelni at gmx.at> wrote:
>> > On Sun, Aug 09, 2009 at 03:10:36AM -0400, Thomas Higdon wrote:
>> >> On Sat, Aug 8, 2009 at 11:21 PM, Mike Melanson<mike at multimedia.cx> wrote:
>> >> > Thomas Higdon wrote:
>> >> >>
>> >> >> On Sat, Aug 8, 2009 at 7:10 PM, Thomas Higdon<thomas.p.higdon at gmail.com>
>> >> >> wrote:
>> >> >>>
>> >> >>> Also, I believe there still may be problems with this demuxer. When
>> >> >>> the stream ends, the video freezes, and A-V sync continues into the
>> >> >>> negative indefinitely. Any ideas on what might be causing this?
>> >> >>
>> >> >> Further investigation reveals that this happens with some other clips
>> >> >> I tried, so perhaps it's just a bug in ffplay.
>> >> >
>> >> > Does FFmpeg work? (E.g., 'ffmpeg -i file.yop %04d.png' should decode the
>> >> > frames to a series of PNG files and exit cleanly.)
>> >>
>> >> As a matter of fact, it didn't (it continued decoding frames
>> >> indefinitely). I've fixed it by returning AVERROR(EIO) in the event
>> >> that an av_get_packet() returns with a size less than I asked for in
>> >> yop_read_packet().
>> >
>> > probabl should be AVERROR_EOF
>>
>> Fixed.
>
> [...]
>> +static int yop_read_header(AVFormatContext *s, AVFormatParameters *ap)
>> +{
>> + ? ?YopDecContext *yop = s->priv_data;
>> + ? ?ByteIOContext *pb = s->pb;
>> +
>> + ? ?AVCodecContext *sound_dec, *video_dec;
>> + ? ?AVStream *sound_stream, *video_stream;
>> +
>> + ? ?int header_data_length;
>> + ? ?int num_frames, frame_rate, num_pal_colors;
>> + ? ?int firstcolor_even, firstcolor_odd;
>> + ? ?int sound_chunk_length;
>> +
>> + ? ?sound_stream = av_new_stream(s, 0);
>> + ? ?video_stream = av_new_stream(s, 1);
>> +
>> + ? ?// Sound
>> + ? ?sound_dec = sound_stream->codec;
>
>> + ? ?sound_dec->codec_type = CODEC_TYPE_AUDIO;
>> + ? ?sound_dec->codec_id = CODEC_ID_ADPCM_IMA_WS;
>
> vertical align like:
> sound_dec->codec_type = CODEC_TYPE_AUDIO;
> sound_dec->codec_id ? = CODEC_ID_ADPCM_IMA_WS;
>
> looks prettier IMHO

Fixed.

>> + ? ?sound_dec->channels = YOP_AUDIO_NUM_CHANNELS;
>> + ? ?sound_dec->sample_rate = YOP_AUDIO_SAMPLE_RATE;
>
> iam not sure but dont you think using the literal values here would
> be clearer?
> sound_dec->channels = 1
> says more, with YOP_AUDIO_NUM_CHANNELS one has to look it up

Others prefer to avoid magic numbers in code, but I'm willing to
accept this if it's generally the convention around here. I've removed
all symbolic constants from this file.

>> +
>> + ? ?// Video
>> + ? ?video_dec = video_stream->codec;
>> + ? ?video_dec->codec_type = CODEC_TYPE_VIDEO;
>> + ? ?video_dec->codec_id = CODEC_ID_YOP;
>> + ? ?video_dec->pix_fmt = PIX_FMT_RGB24;
>> +
>> + ? ?url_fskip(pb, 4);
>> +
>> + ? ?num_frames = get_le16(pb);
>> + ? ?frame_rate = get_byte(pb);
>> + ? ?yop->frame_size = get_byte(pb) * YOP_SECTOR_SIZE;
>> +
>> + ? ?video_dec->width = get_le16(pb);
>> + ? ?video_dec->height = get_le16(pb);
>> +
>> + ? ?yop->num_pal_colors = get_byte(pb);
>> + ? ?firstcolor_odd = get_byte(pb);
>> + ? ?firstcolor_even = get_byte(pb);
>> +
>> + ? ?url_fskip(pb, 3);
>> +
>> + ? ?sound_chunk_length = get_le16(pb);
>> +
>> + ? ?// 1 nibble per sample
>> + ? ?yop->sound_data_length = YOP_AUDIO_SAMPLES_PER_FRAME / 2;
>> +
>
>> + ? ?header_data_length = (size_t)pb->buf_ptr - (size_t)pb->buffer;
>
> hmm
>
>
>> + ? ?url_fskip(pb, YOP_HEADER_LENGTH - header_data_length);
>
> should be url_fseek to an absolute pos i suspect
>

You're right, showing my inexperience with C file I/O. Both fixed.

>> +
>> + ? ?// Frame rate and timestamp info
>
>> + ? ?video_stream->r_frame_rate = av_d2q(frame_rate, 6000);
>
> use of av_d2q() here is misleading
> also why do you have to set r_frame_rate at all?

You're right, it's not necessary.

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

>> +
>> + ? ?yop->stream = 1;
>> +
>> + ? ?return 0;
>> +}
>> +
>> +static int yop_read_packet(AVFormatContext *s, AVPacket *pkt)
>> +{
>> + ? ?YopDecContext *yop = s->priv_data;
>> + ? ?ByteIOContext *pb = s->pb;
>> +
>> + ? ?int ret;
>> + ? ?int skip = 4 + yop->num_pal_colors * 3;
>> +
>> + ? ?AVStream *st;
>
>> + ? ?yop->stream = (yop->stream + 1) % 2;
>
> ^= 1;

Fixed.

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

>> Index: libavformat/yop.h
>> ===================================================================
>> --- libavformat/yop.h (revision 0)
>> +++ libavformat/yop.h (revision 0)
>> @@ -0,0 +1,42 @@
>> +/**
>> + * @file libavformat/yop.h
>> + * Psygnosis YOP header
>> + *
>> + * Copyright (C) 2009 Thomas P. Higdon <thomas.p.higdon at gmail.com>
>> + *
>> + * This file is part of FFmpeg.
>> + *
>> + * FFmpeg is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * FFmpeg is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with FFmpeg; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>> + */
>> +
>> +#ifndef AVFORMAT_YOP_H
>> +#define AVFORMAT_YOP_H
>> +
>> +#define YOP_SIGNATURE "YO"
>> +
>> +#define YOP_AUDIO_SAMPLE_RATE 22050
>> +#define YOP_AUDIO_SAMPLES_PER_FRAME 1840
>> +#define YOP_AUDIO_NUM_CHANNELS 1
>> +
>> +#define YOP_SECTOR_SIZE 2048
>> +#define YOP_HEADER_LENGTH YOP_SECTOR_SIZE
>> +
>
>> +typedef struct yop_dec_context {
>> + ? ?int frame_size;
>> + ? ?int sound_data_length;
>> + ? ?int stream;
>> + ? ?int num_pal_colors;
>> +} YopDecContext;
>> +#endif
>
> that belongs in the demuxer (it says dec so its not common between
> demuxer & muxer)

Fixed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: yop-demux.diff
Type: text/x-patch
Size: 6589 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090811/e6e71484/attachment.bin>