2009/12/27 Michael Niedermayer <michaelni at gmx.at>

On Sat, Dec 26, 2009 at 11:22:15PM +0530, Mohamed Naufal wrote:

...

2009/12/26 Diego Biurrun <diego at biurrun.de> [...] +static int yop_probe(AVProbeData *probe_packet) +{ + if (AV_RB16(probe_packet->buf) == AV_RB16("YO") && + probe_packet->buf[6] && probe_packet->buf[7] && + !(AV_RL16(probe_packet->buf + 8) & 1) && !(AV_RL16(probe_packet->buf + 10) & 1)) + return AVPROBE_SCORE_MAX / 2 + 1;

does this pass probetest?

It does. Were you suggesting returning a higher score? [...]

...

+AVInputFormat yop_demuxer = { + "yop", + NULL_IF_CONFIG_SMALL("Psygnosis YOP Format"), + sizeof(YopDecContext), + yop_probe, + yop_read_header, + yop_read_packet, + .extensions = "yop",

missing code to free packets

Freeing both sound & video packets in read_close() results in a double free when read_packet() fails and packets are freed in it. So can I move all free_packet() calls to read_close() and use flags for selective freeing? [...] Naufal

On Sat, Dec 26, 2009 at 11:22:15PM +0530, Mohamed Naufal wrote:

+ AVPacket sound_packet;

I'd suggest to replace "sound" with "audio" everywhere to be more consistent with the rest of FFmpeg. Also you always immediately return the audio packet, so I don't see why you'd need this at all.

+ video_dec->pix_fmt = PIX_FMT_RGB24;

I don't think you should set this.

+ if ((ret = get_buffer(pb, video_dec->extradata, 8)) != 8) + return -1;

return ret < 0 ? ret : AVERROR_EOF; or so. Also personally I find assignments inside ifs exceedingly bad for readability without any advantage, but that might just be me.

+ yop->stream ^= 1;

I think it might be better to do this without extra state, e.g. just check if video_packet.buffer is != NULL?

+ if (!yop->stream) { + ret = av_new_packet(&yop->video_packet, + yop->frame_size - yop->sound_chunk_length); + if (ret < 0) + return ret; + + yop->video_packet.pos = url_ftell(pb); + ret = get_buffer(pb, yop->video_packet.data, palette_size); + if (ret < 0) { + return ret; + } else if (ret < palette_size) { + yop_free_packets(yop); + return AVERROR_EOF; + }

Please use "goto err_out" instead of duplicating the freeing code.

+ // 1840 samples per frame, 1 nibble per sample; hence 1840/2 = 920 + ret = av_get_packet(pb, &yop->sound_packet, 920); + if (ret < 920) { + yop_free_packets(yop); + return AVERROR_EOF; + }

Please return partial data where ever it makes even remotely sense. The decoder should decide what to do with incomplete/broken data, not the demuxer (since the demuxer just can't do anything sensible about it).

+ url_fskip(pb, yop->sound_chunk_length - 920); + + ret = get_buffer(pb, yop->video_packet.data + palette_size, + actual_video_data_size); + + if (ret < 0) { + return ret; + } else if (ret < actual_video_data_size) { + yop_free_packets(yop); + return AVERROR_EOF; + } + + // arbitrarily return the sound data first + *pkt = yop->sound_packet; + return yop->sound_chunk_length; + } else { + *pkt = yop->video_packet; + pkt->stream_index = 1; + return yop->frame_size - yop->sound_chunk_length; + }

I think this would be more readable as if (yop->stream) { // return buffered packet *pkt = yop->video_packet; pkt->stream_index = 1; return yop->frame_size - yop->sound_chunk_length; } remaining code without "else", saving one indentation level.

On Mon, Dec 28, 2009 at 12:48:51AM +0530, Mohamed Naufal wrote:

2009/12/27 Reimar D?ffinger <Reimar.Doeffinger at gmx.de>

[...]

...

...

+ yop->stream ^= 1;

I think it might be better to do this without extra state, e.g. just check if video_packet.buffer is != NULL?

Did you mean video_packet.data? If so, I don't understand how it can be done since data isn't freed after each read. Could you please elaborate?

you should not keep a copy of the packet once its returned its not "your" packet anymore at this point and this just leads to a memleak and double free [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Its not that you shouldnt use gotos but rather that you should write readable code and code with gotos often but not always is less readable

2009/12/30 Mohamed Naufal <naufal11 at gmail.com>

2009/12/28 Michael Niedermayer <michaelni at gmx.at>

[...]

you should not keep a copy of the packet once its returned

...

its not "your" packet anymore at this point and this just leads to a memleak and double free

I've used av_destruct_packet_nofree(). Hope this will do.

Ping. Naufal

On Wed, Dec 30, 2009 at 08:47:00AM +0530, Mohamed Naufal wrote:

+ if (AV_RB16(probe_packet->buf) == AV_RB16("YO") && + probe_packet->buf[6] && + probe_packet->buf[7] && + !(AV_RL16(probe_packet->buf + 8) & 1) && + !(AV_RL16(probe_packet->buf + 10) & 1))

It's rather pointless to read 16 bits when you only use the value of a single bit.

+ static int ret;

static is not acceptable, that would result in complete chaos when an application decodes two yop files at the same time.

+ if (yop->video_packet.data) { + *pkt = yop->video_packet; + av_destruct_packet_nofree(&yop->video_packet); + pkt->stream_index = 1;

Why not set the stream index to 1 for yop->video_packet and be done with it?

+ return palette_size + ret;

Why not just pkt->size?

+ ret = av_new_packet(&yop->video_packet, + yop->frame_size - yop->audio_block_length); + if (ret < 0) + return ret; + + yop->video_packet.pos = url_ftell(pb); + + ret = get_buffer(pb, yop->video_packet.data, palette_size); + if (ret < 0) { + return ret; + }else if (ret < palette_size) { + av_free_packet(&yop->video_packet); + return AVERROR_EOF; + }

You need to free the packet in both cases since you already allocated it. Of course this would all be much easier if we had the av_append_packet I proposed but some people seem to prefer duplicating this kind of code despite the fact that it's done wrong almost every time.

+ ret = av_get_packet(pb, pkt, 920);

Note that this will set pkt.pos to the start of the audio data, but your demuxer can not start demuxing from that position, thus your demuxer can not support seeking via AVFMT_GENERIC_INDEX - which it IMO should though.

+ ret = get_buffer(pb, yop->video_packet.data + palette_size, + actual_video_data_size); + if (ret < 0) + return ret;

Missing av_shrink_packet for ret < actual_video_data_size.

+ if(yop->video_packet.data) + av_free_packet(&yop->video_packet);

I think the if should not be necessary.

On Thu, Dec 31, 2009 at 05:02:34PM +0530, Mohamed Naufal wrote:

2009/12/31 Reimar D?ffinger <Reimar.Doeffinger at gmx.de>

[...]

...

...

+ ret = av_get_packet(pb, pkt, 920);

Note that this will set pkt.pos to the start of the audio data, but your demuxer can not start demuxing from that position, thus your demuxer can not support seeking via AVFMT_GENERIC_INDEX - which it IMO should though.

Sorry, how do I do that?

Add .flags = AVFMT_GENERIC_INDEX, to yop_demuxer, test seeking (e.g. with ffplay). It should only work reliable if you set pkt.pos to a file position from which your demuxer can start (i.e. the start of the palette data).

+ if (yop->video_packet.data) { + *pkt = yop->video_packet; + yop->video_packet.data = NULL; + yop->video_packet.size = 0;

I'm undecided if this is better or (mis?-)using destruct_nofree.

+ av_free_packet(&yop->video_packet); + av_free_packet(&yop->video_packet); + av_free_packet(&yop->video_packet);

Duplicated quite a few times.

+ ret = get_buffer(pb, yop->video_packet.data + palette_size, + actual_video_data_size); + if (ret < 0) + return ret;

And here you forgot the free. I was quite serious when is suggested to use "goto err_out" to handle the freeing in one single place, if you don't you _will_ have memleaks, I have seen it often enough.

+ else if (ret < actual_video_data_size) + av_shrink_packet(&yop->video_packet, ret);

ret + palette_size

2009/12/31 Mohamed Naufal <naufal11 at gmail.com> [...] Ping Naufal

2010/1/5 Michael Niedermayer <michaelni at gmx.at> [...]

...

+static int yop_read_packet(AVFormatContext *s, AVPacket *pkt) +{ + YopDecContext *yop = s->priv_data; + ByteIOContext *pb = s->pb; + + int ret; + int palette_size = 4 + yop->num_pal_colors * 3;

...

+ int actual_video_data_size = yop->frame_size - + yop->audio_block_length - palette_size;

this still can end up negative, there still are missing checks in the header parsing

Fixed. [...] Naufal

On Wed, 6 Jan 2010 06:46:08 +0100 Michael Niedermayer wrote:

On Tue, Jan 05, 2010 at 11:03:29PM +0530, Mohamed Naufal wrote:

...

2010/1/5 Michael Niedermayer <michaelni at gmx.at>

[...]

...

...

+static int yop_read_packet(AVFormatContext *s, AVPacket *pkt) +{ + YopDecContext *yop = s->priv_data; + ByteIOContext *pb = s->pb; + + int ret; + int palette_size = 4 + yop->num_pal_colors * 3;

...

+ int actual_video_data_size = yop->frame_size - + yop->audio_block_length - palette_size;

this still can end up negative, there still are missing checks in the header parsing

Fixed.

[...]

Naufal

...

Changelog | 1 doc/general.texi | 1 libavformat/Makefile | 1 libavformat/allformats.c | 1 libavformat/avformat.h | 2 libavformat/yop.c | 186 +++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 191 insertions(+), 1 deletion(-) 05b7713388599f9ce9d51388deb0b792ae89051f yop_demux.patch

patch ok if it works (also seeking and no crashes with damaged files)

[...]

libavformat/yop.c: In function ?yop_read_header?: libavformat/yop.c:80: error: ?CODEC_ID_YOP? undeclared (first use in this function) libavformat/yop.c:80: error: (Each undeclared identifier is reported only once libavformat/yop.c:80: error: for each function it appears in.) libavformat/yop.c: In function ?yop_read_packet?: libavformat/yop.c:126: warning: ?av_destruct_packet_nofree? is deprecated (declared at ./libavcodec/avcodec.h:2794) Ben

On Wed, 6 Jan 2010 15:43:33 +0530 Jai Menon wrote:

On Wed, Jan 6, 2010 at 3:39 PM, Benoit Fouet <benoit.fouet at free.fr> wrote:

...

On Wed, 6 Jan 2010 06:46:08 +0100 Michael Niedermayer wrote:

...

On Tue, Jan 05, 2010 at 11:03:29PM +0530, Mohamed Naufal wrote:

...

2010/1/5 Michael Niedermayer <michaelni at gmx.at>

[...]

...

...

+static int yop_read_packet(AVFormatContext *s, AVPacket *pkt) +{ + ? ?YopDecContext *yop = s->priv_data; + ? ?ByteIOContext *pb ?= s->pb; + + ? ?int ret; + ? ?int palette_size ? ? ? ? ? = 4 + yop->num_pal_colors * 3;

...

+ ? ?int actual_video_data_size = yop->frame_size - + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? yop->audio_block_length - palette_size;

this still can end up negative, there still are missing checks in the header parsing

Fixed.

[...]

Naufal

...

?Changelog ? ? ? ? ? ? ? ?| ? ?1 ?doc/general.texi ? ? ? ? | ? ?1 ?libavformat/Makefile ? ? | ? ?1 ?libavformat/allformats.c | ? ?1 ?libavformat/avformat.h ? | ? ?2 ?libavformat/yop.c ? ? ? ?| ?186 +++++++++++++++++++++++++++++++++++++++++++++++ ?6 files changed, 191 insertions(+), 1 deletion(-) 05b7713388599f9ce9d51388deb0b792ae89051f ?yop_demux.patch

patch ok if it works (also seeking and no crashes with damaged files)

[...]

libavformat/yop.c: In function ?yop_read_header?: libavformat/yop.c:80: error: ?CODEC_ID_YOP? undeclared (first use in this function) libavformat/yop.c:80: error: (Each undeclared identifier is reported only once libavformat/yop.c:80: error: for each function it appears in.) libavformat/yop.c: In function ?yop_read_packet?: libavformat/yop.c:126: warning: ?av_destruct_packet_nofree? is deprecated (declared at ./libavcodec/avcodec.h:2794)

Now that the demuxer is approved, Naufal should go ahead and submit the decoder. Also for game formats etc, i think its better if playback support is added as one single commit.

at least the decoder should be submitted first so that build is not broken :) I've quickly tested the yop files available in the samples directory; they do not crash, the audio plays ok, but seek does not work. Ben

2010/1/6 Benoit Fouet <benoit.fouet at free.fr> [...] at least the decoder should be submitted first so that build is not broken

:) I've quickly tested the yop files available in the samples directory; they do not crash, the audio plays ok, but seek does not work.

[...] I think seeking works now. Have set PKT_FLAG_KEY for every packet but not sure this is the proper method. Have also included the decoder code. Naufal

On 15 January 2010 05:41, Michael Niedermayer <michaelni at gmx.at> wrote: [...]

...

+typedef struct YopDecContext { + AVFrame frame; + AVCodecContext *avctx; + AVPacket *avpkt; + + int num_pal_colors; + int first_color[2]; + int tag_needs_next_byte;

...

+ int current_tag_byte;

you dont need that variable

If I don't increment s->srcptr in the if block of yop_get_next_nibble(), color_num in yop_paint_block() can have an incorrect value. [...]

...

+/** + * Update the palette based on the frame number. Consume all of the palette + * bytes on the input. + */ +static void yop_update_palette(YopDecContext *s) +{ + int firstcolor; + int i, j; + + firstcolor = s->first_color[s->avctx->frame_number & 1];

this will be wrong with seeking id suspect

How else do I do it? Couldn't post earlier. My laptop got stolen :( Naufal

On 22 February 2010 01:40, Michael Niedermayer <michaelni at gmx.at> wrote:

On Thu, Feb 18, 2010 at 05:35:59PM +0530, Mohamed Naufal wrote:

...

On 15 January 2010 05:41, Michael Niedermayer <michaelni at gmx.at> wrote:

[...]

...

...

+typedef struct YopDecContext { + AVFrame frame; + AVCodecContext *avctx; + AVPacket *avpkt; + + int num_pal_colors; + int first_color[2]; + int tag_needs_next_byte;

...

+ int current_tag_byte;

you dont need that variable

If I don't increment s->srcptr in the if block of yop_get_next_nibble(), color_num in yop_paint_block() can have an incorrect value.

you dont need the current_tag_byte variable

What I meant to say was if incrementing s->srcptr is postponed to the else block and yop_paint_block is called immediately after the if block, the local variable color_num = *(s->srcptr + paint_lut[tag][i]) will have an incorrect value. Therefore s->srcptr is incremented in the if block and the original value stored in current_tag_byte. [...]

...

...

+ firstcolor = s->first_color[s->avctx->frame_number & 1];

this will be wrong with seeking id suspect

How else do I do it?

Either the demuxer marks only even frames as keyframe then the decoder can when it detects seeking start counting from even or the demuxer has to indicate if a frame is even or odd to the decoder also there is the question if they are keyframes or not, you obviosuly should not mark then incorrectly

Now checked using (pkt->pos - HDR_SIZE) / frame_size & 1 Hope this is OK.

[...]

...

+ s->row_pos = 0; + + while ((size_t) s->dstptr - (size_t) s->dstbuf < + avctx->width * RGB_COLORS * avctx->height && + (size_t) s->srcptr - (size_t) avpkt->data < avpkt->size) { + current_nibble = yop_get_next_nibble(s);

useless casts

Fixed.

[...]

...

+AVInputFormat yop_demuxer = { + "yop", + NULL_IF_CONFIG_SMALL("Psygnosis YOP Format"), + sizeof(YopDecContext), + yop_probe, + yop_read_header, + yop_read_packet, + yop_read_close, + .extensions = "yop", + .flags = AVFMT_GENERIC_INDEX, +};

thats missing code to flush the buffered packet in case of seeking

Don't the buffered packets get freed in av_seek_frame_generic()?. I must be missing something. Anyway, the demuxer now uses a custom seeking function and seeking works without any problems. Naufal

On Tue, Mar 09, 2010 at 12:27:52AM +0530, Mohamed Naufal wrote:

On 22 February 2010 01:40, Michael Niedermayer <michaelni at gmx.at> wrote:

...

On Thu, Feb 18, 2010 at 05:35:59PM +0530, Mohamed Naufal wrote:

...

On 15 January 2010 05:41, Michael Niedermayer <michaelni at gmx.at> wrote:

[...]

...

...

+typedef struct YopDecContext { + AVFrame frame; + AVCodecContext *avctx; + AVPacket *avpkt; + + int num_pal_colors; + int first_color[2]; + int tag_needs_next_byte;

...

+ int current_tag_byte;

you dont need that variable

If I don't increment s->srcptr in the if block of yop_get_next_nibble(), color_num in yop_paint_block() can have an incorrect value.

you dont need the current_tag_byte variable

What I meant to say was if incrementing s->srcptr is postponed to the else block and yop_paint_block is called immediately after the if block, the local variable color_num = *(s->srcptr + paint_lut[tag][i]) will have an incorrect value. Therefore s->srcptr is incremented in the if block and the original value stored in current_tag_byte.

you do not need the current_tag_byte variable, iam not speaking about any increasing of any pointers

[...]

...

...

...

+ firstcolor = s->first_color[s->avctx->frame_number & 1];

this will be wrong with seeking id suspect

How else do I do it?

Either the demuxer marks only even frames as keyframe then the decoder can when it detects seeking start counting from even or the demuxer has to indicate if a frame is even or odd to the decoder also there is the question if they are keyframes or not, you obviosuly should not mark then incorrectly

Now checked using (pkt->pos - HDR_SIZE) / frame_size & 1 Hope this is OK.

no, pos can be arbitrary after remuxing, you cannot use its value in the decoder

[...]

...

[...]

...

+AVInputFormat yop_demuxer = { + "yop", + NULL_IF_CONFIG_SMALL("Psygnosis YOP Format"), + sizeof(YopDecContext), + yop_probe, + yop_read_header, + yop_read_packet, + yop_read_close, + .extensions = "yop", + .flags = AVFMT_GENERIC_INDEX, +};

thats missing code to flush the buffered packet in case of seeking

Don't the buffered packets get freed in av_seek_frame_generic()?. I must be missing something. Anyway, the demuxer now uses a custom seeking function and seeking works without any problems.

you still aren freeing the packet, the generic code cannot free your privately allocatd packet [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 2 "100% positive feedback" - "All either got their money back or didnt complain" "Best seller ever, very honest" - "Seller refunded buyer after failed scam"

On Wed, Mar 17, 2010 at 12:52:42PM +0530, Mohamed Naufal wrote:

On 16 March 2010 06:25, Michael Niedermayer <michaelni at gmx.at> wrote:

[...]

...

you move the increasing to the else, this breaks the code you dont have to move it there to remove current_tab_byte

Please find the updated patch attached.

[...]

+/** + * Lookup table for copying macroblocks. Each entry contains the respective + * x and y pixel offset for the copy source. + */ +static const int8_t motion_vector[16][2] = {{-4, -4}, + {-2, -4}, + { 0, -4}, + { 2, -4}, + {-4, -2}, + {-4, 0}, + {-3, -3}, + {-1, -3}, + { 1, -3}, + { 3, -3}, + {-3, -1}, + {-2, -2}, + { 0, -2}, + { 2, -2}, + { 4, -2}, + {-2, 0}, + }; + +static av_cold int yop_decode_init(AVCodecContext *avctx) +{ + YopDecContext *s = avctx->priv_data; + + s->avctx = avctx; + + if (avctx->width & 1 || avctx->height & 1 || + avcodec_check_dimensions(avctx, avctx->width, avctx->height)) { + av_log(avctx, AV_LOG_ERROR, "YOP has invalid dimensions\n"); + return -1; + } + + avctx->pix_fmt = PIX_FMT_RGB24;

doesnt the decoder work with PAL8 ? [...]

+static int yop_decode_frame(AVCodecContext *avctx, void *data, int *data_size, + AVPacket *avpkt) +{ + YopDecContext *s = avctx->priv_data; + uint8_t current_byte = 0; + + if (!s->frame.data[0]) { + s->frame.data[0] = av_mallocz(s->frame.linesize[0] * avctx->height); + if (!s->frame.data[0]) + return AVERROR(ENOMEM); + + s->frame.base[0] = s->frame.data[0]; + s->frame.key_frame = 1; + } + + s->srcptr = avpkt->data + 4; + s->dstbuf = s->frame.data[0]; + s->dstptr = s->frame.data[0]; + s->row_pos = 0; + + // Copy the frame palette into our struct + yop_update_palette(s, !(avpkt->flags & PKT_FLAG_KEY)); + + while (s->dstptr - s->dstbuf < + avctx->width * RGB_COLORS * avctx->height && + s->srcptr - avpkt->data < avpkt->size) { + + yop_get_next_nibble(s, ¤t_byte); +

+ if ((current_byte & 0xf0) != 0xf0) { + yop_paint_block(s, current_byte >> 4); + }else { + yop_get_next_nibble(s, ¤t_byte); + yop_copy_previous_block(s, current_byte >> 4); + }

extracting the top 4bits is done several times [...]

+static int yop_read_packet(AVFormatContext *s, AVPacket *pkt) +{ + YopDecContext *yop = s->priv_data; + ByteIOContext *pb = s->pb; + + int ret; + int palette_size = 4 + yop->num_pal_colors * 3; + int actual_video_data_size = yop->frame_size - + yop->audio_block_length - palette_size; + + yop->video_packet.stream_index = 1; + + if (yop->video_packet.data) { + *pkt = yop->video_packet; + yop->video_packet.data = NULL; + yop->video_packet.size = 0; + + if (!yop->odd_frame) + pkt->flags |= PKT_FLAG_KEY; [...] +static int yop_read_seek(AVFormatContext *s, int stream_index, + int64_t timestamp, int flags) +{ + YopDecContext *yop = s->priv_data; + int64_t frame_pos, pos_min, pos_max; + + av_free_packet(&yop->video_packet); + + pos_min = s->data_offset; + pos_max = url_fsize(s->pb) - yop->frame_size; + + if(!stream_index) + return -1; + + frame_pos = timestamp * yop->frame_size + pos_min; + yop->odd_frame = timestamp & 1;

this code is a bit inconsistent. are half of the frames really more keyframe than the other half? if not you cannot set keyframe for one half different than the other. if they are you cannot seek to one half [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire

On Thu, Mar 18, 2010 at 05:42:13AM +0530, Mohamed Naufal wrote:

On 18 March 2010 05:01, Michael Niedermayer <michaelni at gmx.at> wrote:

[...]

...

...

+ + ? ? ? ?yop_get_next_nibble(s, ¤t_byte); +

...

+ ? ? ? ?if ((current_byte & 0xf0) != 0xf0) { + ? ? ? ? ? ?yop_paint_block(s, current_byte >> 4); + ? ? ? ?}else { + ? ? ? ? ? ?yop_get_next_nibble(s, ¤t_byte); + ? ? ? ? ? ?yop_copy_previous_block(s, current_byte >> 4); + ? ? ? ?}

extracting the top 4bits is done several times

I can't think of any other way to eliminate the current_tag_byte variable in the previous patch :(

something like: yop_get_next_nibble(){ if(low_nibble){ ret= *low_nibble & 0xF; low_nibble=NULL; }else{ low_nibble=p; ret= *p++>>4; } return ret; } might work, not really pretty either

...

[...]

...

+static int yop_read_packet(AVFormatContext *s, AVPacket *pkt) +{ + ? ?YopDecContext *yop = s->priv_data; + ? ?ByteIOContext *pb ?= s->pb; + + ? ?int ret; + ? ?int palette_size ? ? ? ? ? = 4 + yop->num_pal_colors * 3; + ? ?int actual_video_data_size = yop->frame_size - + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? yop->audio_block_length - palette_size; + + ? ?yop->video_packet.stream_index = 1; + + ? ?if (yop->video_packet.data) { + ? ? ? ?*pkt ? ? ? ? ? ? ? ? ? = ?yop->video_packet; + ? ? ? ?yop->video_packet.data = ?NULL; + ? ? ? ?yop->video_packet.size = ?0; + + ? ? ? ?if (!yop->odd_frame) + ? ? ? ? ? ?pkt->flags |= PKT_FLAG_KEY; [...] +static int yop_read_seek(AVFormatContext *s, int stream_index, + ? ? ? ? ? ? ? ? ? ? ? ? int64_t timestamp, int flags) +{ + ? ?YopDecContext *yop = s->priv_data; + ? ?int64_t frame_pos, pos_min, pos_max; + + ? ?av_free_packet(&yop->video_packet); + + ? ?pos_min = s->data_offset; + ? ?pos_max = url_fsize(s->pb) - yop->frame_size; + + ? ?if(!stream_index) + ? ? ? ?return -1; + + ? ?frame_pos ? ? ?= timestamp * yop->frame_size + pos_min; + ? ?yop->odd_frame = timestamp & 1;

this code is a bit inconsistent. are half of the frames really more keyframe than the other half? if not you cannot set keyframe for one half different than the other.

Can I set the flag to some other value? If not, is there some other way the demuxer can tell the decoder if a frame is odd/even?

you could put a 0 or 1 in AVPacket.data[0] but this is ugly, so lets just keep the ugly marking of half of them as keyframes.

...

if they are you cannot seek to one half

Shouldn't frame_pos = timestamp * frame_size + pos_min; seek(frame_pos); take care of that?

that looks like it can seek to any frame, not just the ones marked as keyframe [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 3 "Rare item" - "Common item with rare defect or maybe just a lie" "Professional" - "'Toy' made in china, not functional except as doorstop" "Experts will know" - "The seller hopes you are not an expert"

On Fri, Mar 26, 2010 at 05:20:29AM +0530, Mohamed Naufal wrote:

On 24 March 2010 07:51, Michael Niedermayer <michaelni at gmx.at> wrote:

[...]

...

you could put a 0 or 1 in AVPacket.data[0] but this is ugly, so lets just keep the ugly marking of half of them as keyframes.

The demuxer now marks pkt.data[0] as odd/even and all pkt.flags as key-frames. Doing this so that seeking using

frame_pos = timestamp * frame_size + pos_min; seek(frame_pos);

wouldn't be wrong. The first 4 B of pkt.data is unused anyway.

Also modified the decoder to output in PAL8.

Naufal [...] +/** + * Paint a macroblock using the pattern in paint_lut. + * @param s codec context + * @param tag The tag that was in the nibble. + */ +static void yop_paint_block(YopDecContext *s, int tag) +{ + int offset, color_num, i; + + for (i = 0; i < 4; i++) { + offset = s->block_offset_lut[i]; + color_num = *(s->srcptr + paint_lut[tag][i]); + *(s->dstptr + offset) = color_num; + }

you dot need the color_num nor offset variables and it probably makes sense to unroll this loop as its rather speed relevant and small

+ // The number of src bytes consumed is in the last part of the lut entry. + s->srcptr += paint_lut[tag][4]; +} + +/** + * Copy a previously painted macroblock to the current_block. + * @param copy_tag The tag that was in the nibble. + */ +static void yop_copy_previous_block(YopDecContext *s, int copy_tag) +{ + uint8_t *bufptr; + int offset, i; + + // Calculate position for the copy source + bufptr = s->dstptr + motion_vector[copy_tag][0] + + s->frame.linesize[0] * motion_vector[copy_tag][1]; +

+ for (i = 0; i < 4; i++) { + offset = s->block_offset_lut[i]; + *(s->dstptr + offset) = *(bufptr + offset); + }

s->dstptr[offset] looks nicer and this loop too probably should be unrolled

+} + +/** + * Return the next nibble in sequence, consuming a new byte on the input + * only if necessary. + */ +static uint8_t yop_get_next_nibble(YopDecContext *s) +{ + int ret; + + if (s->low_nibble) { + ret = *s->low_nibble & 0xf; + s->low_nibble = NULL; + }else { + s->low_nibble = s->srcptr++; + ret = *s->low_nibble >> 4; + } + return ret; +} +

+/** + * Take s->dstptr to the next macroblock in sequence. + */ +static void yop_next_macroblock(YopDecContext *s) +{ + // If we are advancing to the next row of macroblocks + if (s->row_pos == s->frame.linesize[0] - 2) { + s->dstptr += s->frame.linesize[0];

this looks like it assumes width == linesize this is not guranteed by get_buffer() [...]

+ palette_size = 4 + yop->num_pal_colors * 3; [...] + int palette_size = 4 + yop->num_pal_colors * 3;

duplicate [...]

+static int yop_read_seek(AVFormatContext *s, int stream_index, + int64_t timestamp, int flags) +{ + YopDecContext *yop = s->priv_data; + int64_t frame_pos, pos_min, pos_max; + + av_free_packet(&yop->video_packet); + + pos_min = s->data_offset; + pos_max = url_fsize(s->pb) - yop->frame_size; + + if(!stream_index) + return -1; + + frame_pos = timestamp * yop->frame_size + pos_min; + yop->odd_frame = timestamp & 1; + + if (frame_pos < pos_min) { + frame_pos = pos_min; + yop->odd_frame = 0; + }else if (frame_pos > pos_max) { + frame_pos = pos_max; + yop->odd_frame = (pos_max - pos_min) / + yop->frame_size & 1; + }

cliping timestamp instead of pos would make the special case handling of odd_frame unneeded [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I wish the Xiph folks would stop pretending they've got something they do not. Somehow I fear this will remain a wish. -- M?ns Rullg?rd

On Sat, Mar 27, 2010 at 01:18:51AM +0530, Mohamed Naufal wrote:

On 26 March 2010 21:06, Michael Niedermayer <michaelni at gmx.at> wrote:

[...]

...

...

+ * Take s->dstptr to the next macroblock in sequence. + */ +static void yop_next_macroblock(YopDecContext *s) +{ + ? ?// If we are advancing to the next row of macroblocks + ? ?if (s->row_pos == s->frame.linesize[0] - 2) { + ? ? ? ?s->dstptr ?+= s->frame.linesize[0];

this looks like it assumes width == linesize this is not guranteed by get_buffer()

But width is assigned to linesize after get_buffer().

not pretty but i see this probably isnt easy to avoid did you test the code with damaged files (tools/trasher) it shouldnt crash or hang with it except that its probably ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The bravest are surely those who have the clearest vision of what is before them, glory and danger alike, and yet notwithstanding go out to meet it. -- Thucydides

On Sat, Mar 27, 2010 at 07:08:11AM +0530, Mohamed Naufal wrote:

On 27 March 2010 06:35, Michael Niedermayer <michaelni at gmx.at> wrote:

...

On Sat, Mar 27, 2010 at 01:18:51AM +0530, Mohamed Naufal wrote:

...

On 26 March 2010 21:06, Michael Niedermayer <michaelni at gmx.at> wrote:

[...]

...

...

+ * Take s->dstptr to the next macroblock in sequence. + */ +static void yop_next_macroblock(YopDecContext *s) +{ + ? ?// If we are advancing to the next row of macroblocks + ? ?if (s->row_pos == s->frame.linesize[0] - 2) { + ? ? ? ?s->dstptr ?+= s->frame.linesize[0];

this looks like it assumes width == linesize this is not guranteed by get_buffer()

But width is assigned to linesize after get_buffer().

not pretty but i see this probably isnt easy to avoid

did you test the code with damaged files (tools/trasher) it shouldnt crash or hang with it

except that its probably ok

Finally! ;) Checked. Works without problems. Also would it be appropriate to add my name to copyright?

i think so, yes [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB There will always be a question for which you do not know the correct awnser.

Mohamed Naufal <naufal11 <at> gmail.com> writes:

Patch attached.

AFAICT, the aspect ratio is wrong. Test for example ffmpeg -i CIMOVI03.YOP test.avi ffmpeg -i CIMOVI03.YOP -s 580x348 test2.avi The second sample looks more correct (ffmpeg reports "Video: yop, pal8, 580x174, 12 fps, 12 tbr, 12 tbn, 12 tbc" for the original file), but I don't know what the exact ratio is. Please look at mobygames to compare. Carl Eugen

On 27 March 2010 16:35, Mohamed Naufal <naufal11 at gmail.com> wrote:

On 27 March 2010 15:55, Carl Eugen Hoyos <cehoyos at ag.or.at> wrote:

...

Mohamed Naufal <naufal11 <at> gmail.com> writes:

...

Patch attached.

AFAICT, the aspect ratio is wrong. Test for example ffmpeg -i CIMOVI03.YOP test.avi ffmpeg -i CIMOVI03.YOP -s 580x348 test2.avi The second sample looks more correct (ffmpeg reports "Video: yop, pal8, 580x174, 12 fps, 12 tbr, 12 tbn, 12 tbc" for the original file), but I don't know what the exact ratio is. Please look at mobygames to compare.

But http://samples.mplayerhq.hu/game-formats/yop/yop.txt says that the sample at http://samples.mplayerhq.hu/game-formats/yop/CIMOVI01.avi is a reference decoder output and it has the aspect ratio 580x174 too.

Naufal

Ah, I checked out the screenshot at mobygames. Will post the updated patch in a bit. Will duplicating each line width wise do? Naufal

On Sat, Mar 27, 2010 at 07:39:14AM +0530, Mohamed Naufal wrote:

+static const uint8_t paint_lut[15][5] = { {0, 1, 2, 3, 4}, + {0, 1, 2, 0, 3}, + {0, 1, 2, 1, 3}, + {0, 1, 2, 2, 3}, + {0, 1, 0, 2, 3}, + {0, 1, 0, 0, 2}, + {0, 1, 0, 1, 2}, + {0, 1, 1, 2, 3}, + {0, 0, 1, 2, 3}, + {0, 0, 1, 0, 2}, + {0, 1, 1, 0, 2}, + {0, 0, 1, 1, 2}, + {0, 0, 0, 1, 2}, + {0, 0, 0, 0, 1}, + {0, 1, 1, 1, 2}, + };

First byte is always 0, removing it will simplify address calculations. It _might_ also be faster to e.g. compress all 4 values into e.g. a single uint16_t value, but probably not worth the effort. I do think it would look nicer if you started the data on a new line with 4 spaces indentation and e.g. 2 {} blocks per line instead of having them all the way on the right side.

+ avcodec_check_dimensions(avctx, avctx->width, avctx->height)) {

Only return values < 0 indicate an error, > 0 in FFmpeg does not usually indicate an error.

+ s->num_pal_colors = AV_RL8(avctx->extradata); + s->first_color[0] = AV_RL8(avctx->extradata + 1); + s->first_color[1] = AV_RL8(avctx->extradata + 2);

Obfuscation. avctx->extradata[0] avctx->extradata[1] avctx->extradata[2]

+/** + * Copy a previously painted macroblock to the current_block. + * @param copy_tag The tag that was in the nibble. + */ +static void yop_copy_previous_block(YopDecContext *s, int copy_tag) +{ + uint8_t *bufptr; + + // Calculate position for the copy source + bufptr = s->dstptr + motion_vector[copy_tag][0] + + s->frame.linesize[0] * motion_vector[copy_tag][1];

How do you know bufptr is a valid pointer?

+ if (avctx->get_buffer(avctx, &s->frame) < 0) { + av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n"); + return -1; + } + + s->frame.linesize[0] = avctx->width;

I strongly object to this, this is outside of any documented API. The CODEC_CAP_DR1 documentation indicates that you must set it if you use get_buffer, but changing linesize is incompatible with DR. You either need to respect the linesize as it is or do the buffer management without get_buffer. Or, if we really want this to be a valid way of doing it, improve that CODEC_CAP_DR1 documentation and add a comment that this codec intentionally does not set CODEC_CAP_DR1 because it can not work with it.

+ palette = (uint32_t *)s->frame.data[1]; + + for (i = 0; i < s->num_pal_colors; i++, s->srcptr += 3) + palette[i + firstcolor] = (s->srcptr[0] << 18) | + (s->srcptr[1] << 10) | + (s->srcptr[2] << 2);

I think you should initialize the whole palette area to some value, even the unused parts.

+ // Reset this so the next frame doesn't use a stale tag. + s->low_nibble = NULL;

Wouldn't it make more sense to initialize this together with srcptr instead?

On Sat, Mar 27, 2010 at 01:15:46PM +0100, Michael Niedermayer wrote:

On Sat, Mar 27, 2010 at 01:00:38PM +0100, Reimar D?ffinger wrote:

...

you use get_buffer, but changing linesize is incompatible with DR. You either need to respect the linesize as it is or do the buffer management without get_buffer.

if CODEC_CAP_DR1 is not set get_buffer == our internal get_buffer

That's what I though first was the definition of CODEC_CAP_DR1, but when I looked it up the documentation said something else. But I now found that the get_buffer documentation mandates this behaviour so I guess it is ok from that aspect. I'm not 100% sure there will not be any issues since the line alignment might be less than what we usually promise, but if there's no easy way to fix it I withdraw my objection.

...

Or, if we really want this to be a valid way of doing it, improve that CODEC_CAP_DR1 documentation and add a comment that this codec intentionally does not set CODEC_CAP_DR1 because it can not work with it.

our documentation is shit but you coul interpret it as CODEC_CAP_DR1 -> codec uses get_buffer but not neccassarily <-

Why not add a "makes pigs fly. Or maybe not.", that's about as useful :-) Hopefully what I committed makes it clearer.

On 27 March 2010 19:03, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:

On Sat, Mar 27, 2010 at 01:15:46PM +0100, Michael Niedermayer wrote:

...

anyway it would be great if we could avoid the linesize overriding but i dont think this is possible with this codec, i think it depends on having line stored with linesize==width (or it would need some messy special case handling close to the image borders, which does not seem like a good idea)

Btw. I suspect it is only significantly more messy than the current solution because the current patch cheats and lacks necessary checks, happily crashing with invalid motion vectors near the start and end of frame.

[...] But the motion vector doesn't cause the crossing of the upper frame boundary. Fixed other iss

On 27 March 2010 22:37, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:

On Sat, Mar 27, 2010 at 09:52:48PM +0530, Mohamed Naufal wrote:

...

On 27 March 2010 19:03, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:

...

On Sat, Mar 27, 2010 at 01:15:46PM +0100, Michael Niedermayer wrote:

...

anyway it would be great if we could avoid the linesize overriding but i dont think this is possible with this codec, i think it depends on having line stored with linesize==width (or it would need some messy special case handling close to the image borders, which does not seem like a good idea)

Btw. I suspect it is only significantly more messy than the current solution because the current patch cheats and lacks necessary checks, happily crashing with invalid motion vectors near the start and end of frame.

[...] But the motion vector doesn't cause the crossing of the upper frame boundary.

I don't quite understand that part, but I think I now see the issue, I guess it could be that bufptr[0] is at the end of one and bufptr[1] and the start of the next row. Then I agree it's probably better to leave the linesize hack in.

...

+ ? ?// Calculate position for the copy source + ? ?bufptr = s->dstptr + motion_vector[copy_tag][0] + + ? ? ? ? ? ? s->frame.linesize[0] * motion_vector[copy_tag][1]; + ? ?if (bufptr < s->dstbuf) { + ? ? ? ?av_log(s->avctx, AV_LOG_ERROR, + ? ? ? ? ? ? ? "YOP: cannot decode, file probably corrupt"); + ? ? ? ?return AVERROR_INVALIDDATA; + ? ?}

I guess this is mostly ok here, but you should be aware that this is not really valid C, and in the real world can have issues with overflows (in particular it assumes there is a NULL-page larger than 4*linesize[0]). Btw. (I'm not asking you to implement it) you could probably gain quite some speed by using a "tighter" loop for the middle part of the frame that does not do this check and the dstptr - dstbuf check, and thanks to padding the srcptr check could be done much more rarely there as well.

But wouldn't that entail further checks? Please elaborate. Thanks, Naufal

On 27 March 2010 23:19, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:

On Sat, Mar 27, 2010 at 10:57:30PM +0530, Mohamed Naufal wrote:

...

On 27 March 2010 22:37, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:

...

Btw. (I'm not asking you to implement it) you could probably gain quite some speed by using a "tighter" loop for the middle part of the frame that does not do this check and the dstptr - dstbuf check, and thanks to padding the srcptr check could be done much more rarely there as well.

But wouldn't that entail further checks? Please elaborate.

Checks that you are "in the middle of the frame", yes. But it's sufficient to do that check e.g. only after each 8 bytes decoded. So you could decode 8 bytes in a row without any checks, then do all the checks necessary and then either continue with the "fast" code or switch to the "careful" code. Yes, you will have more checks, but those checks will be executed only 1/8th as often.

Could the current patch be okayed? Will look at speed improvements in a bit. Thanks, Naufal

On 28 March 2010 01:27, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:

On Sun, Mar 28, 2010 at 01:22:08AM +0530, Mohamed Naufal wrote:

...

Could the current patch be okayed? Will look at speed improvements in a bit.

I have no objections... _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel at mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

Could someone commit it then? Thanks, Naufal

On Sun, Mar 28, 2010 at 04:05:37PM +0200, Stefano Sabatini wrote:

On date Sunday 2010-03-28 03:02:34 +0530, Mohamed Naufal encoded:

...

On 28 March 2010 01:27, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:

...

On Sun, Mar 28, 2010 at 01:22:08AM +0530, Mohamed Naufal wrote:

...

Could the current patch be okayed? Will look at speed improvements in a bit.

I have no objections... _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel at mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

Could someone commit it then?

Any other objections?

Michael, OK to apply?

didnt i alraedy ok a previous version? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Observe your enemies, for they first find out your faults. -- Antisthenes

On 30 March 2010 03:47, Stefano Sabatini <stefano.sabatini-lala at poste.it> wrote:

On date Monday 2010-03-29 03:01:20 +0200, Michael Niedermayer encoded:

...

On Sun, Mar 28, 2010 at 04:05:37PM +0200, Stefano Sabatini wrote:

...

On date Sunday 2010-03-28 03:02:34 +0530, Mohamed Naufal encoded:

...

On 28 March 2010 01:27, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:

...

On Sun, Mar 28, 2010 at 01:22:08AM +0530, Mohamed Naufal wrote:

...

Could the current patch be okayed? Will look at speed improvements in a bit.

I have no objections... [...] Could someone commit it then?

Any other objections?

Michael, OK to apply?

didnt i alraedy ok a previous version?

So applied. Congratulations and thanks Mohamed for the contribution!

Thank you! I'd like to specially thank Michael for his patience ;) and Jai for opening the gates for me. Naufal

On 27 March 2010 19:03, Reimar D?ffinger <Reimar.Doeffinger at gmx.de> wrote:

On Sat, Mar 27, 2010 at 01:15:46PM +0100, Michael Niedermayer wrote:

...

anyway it would be great if we could avoid the linesize overriding but i dont think this is possible with this codec, i think it depends on having line stored with linesize==width (or it would need some messy special case handling close to the image borders, which does not seem like a good idea)

Btw. I suspect it is only significantly more messy than the current solution because the current patch cheats and lacks necessary checks, happily crashing with invalid motion vectors near the start and end of frame.

But the motion vector doesn't cause the crossing of the upper frame boundary. Fixed other issues. Naufal

On date Saturday 2010-03-27 07:39:14 +0530, Mohamed Naufal encoded: [...]

+static av_cold int yop_decode_init(AVCodecContext *avctx) +{ + YopDecContext *s = avctx->priv_data; + s->avctx = avctx; + + if (avctx->width & 1 || avctx->height & 1 || + avcodec_check_dimensions(avctx, avctx->width, avctx->height)) { + av_log(avctx, AV_LOG_ERROR, "YOP has invalid dimensions\n"); + return -1; + } + + avctx->pix_fmt = PIX_FMT_PAL8; + + s->num_pal_colors = AV_RL8(avctx->extradata); + s->first_color[0] = AV_RL8(avctx->extradata + 1); + s->first_color[1] = AV_RL8(avctx->extradata + 2); + + if (s->num_pal_colors + s->first_color[0] > 256 || + s->num_pal_colors + s->first_color[1] > 256) { + av_log(avctx, AV_LOG_ERROR, + "YOP: palette parameters invalid, header probably corrupt\n"); + return -1;

Now that we have saner error codes let's start to use them. This could be AVERROR_INVALIDDATA. [...]

+static int yop_decode_frame(AVCodecContext *avctx, void *data, int *data_size, + AVPacket *avpkt) +{ + YopDecContext *s = avctx->priv_data; + int tag, firstcolor, is_odd_frame, i; + uint32_t *palette; + uint8_t const *dstbuf; + + if (s->frame.data[0]) + avctx->release_buffer(avctx, &s->frame); + + if (avctx->get_buffer(avctx, &s->frame) < 0) { + av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n"); + return -1; + }

if ((ret = avctx->get_buffer(avctx, &s->frame) < 0) { ... return ret; } Regards. -- FFmpeg = Faithful & Forgiving Multipurpose Pure Eretic Geisha

On date Saturday 2010-03-27 15:10:01 +0100, Reimar D?ffinger encoded:

On Sat, Mar 27, 2010 at 03:05:34PM +0100, Stefano Sabatini wrote:

...

On date Saturday 2010-03-27 07:39:14 +0530, Mohamed Naufal encoded:

...

+ if (avctx->get_buffer(avctx, &s->frame) < 0) { + av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n"); + return -1; + }

if ((ret = avctx->get_buffer(avctx, &s->frame) < 0) {

Could we please get those assignments in ifs banned? In 99% of cases they only make the code harder to read without even the slightest advantage I can see.

They save you a line of code, good if you're a programmer/golf-player and like to impress your friends with very compact code ;-). And this is a rather bikesheddish issue, I suggest to just let the code writer/maintainer choose the style which suits her better, at least for this particular idiomatic syntax. Regards. -- FFmpeg = Faithless and Fancy Merciless Problematic Epic Genius

On Sat, Mar 27, 2010 at 03:10:01PM +0100, Reimar D?ffinger wrote:

On Sat, Mar 27, 2010 at 03:05:34PM +0100, Stefano Sabatini wrote:

...

On date Saturday 2010-03-27 07:39:14 +0530, Mohamed Naufal encoded:

...

+ if (avctx->get_buffer(avctx, &s->frame) < 0) { + av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n"); + return -1; + }

if ((ret = avctx->get_buffer(avctx, &s->frame) < 0) {

Could we please get those assignments in ifs banned? In 99% of cases they only make the code harder to read without even the slightest advantage I can see.

i dont mind a patch to patcheck that suggests this not to be done otherwise i agree with the others that the author/maintainer should decide such style issues [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Republics decline into democracies and democracies degenerate into despotisms. -- Aristotle

On Wed, 06 Jan 2010 14:32:42 +0100 Diego Biurrun wrote:

On Wed, Jan 06, 2010 at 11:09:03AM +0100, Benoit Fouet wrote:

...

On Wed, 6 Jan 2010 06:46:08 +0100 Michael Niedermayer wrote:

...

patch ok if it works (also seeking and no crashes with damaged files)

libavformat/yop.c: In function ?yop_read_header?: libavformat/yop.c:80: error: ?CODEC_ID_YOP? undeclared (first use in this function) libavformat/yop.c:80: error: (Each undeclared identifier is reported only once libavformat/yop.c:80: error: for each function it appears in.)

Run 'make config'.

there is no yop decoder, running 'make config' won't make it appear magically ;) (I already added the codec ID to test the demuxer though...) Ben