[FFmpeg-devel] [PATCH] Psygnosis YOP demuxer

[Michael Niedermayer]

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


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


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


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


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


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


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


[...]
> 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)

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

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/20090810/0564e119/attachment.pgp>