ARMovie/RPL demuxer rev3

Per subject, ARMovie/RPL demuxer; I think this addresses all
the
review comments.

-Eli

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Thu, Mar 27, 2008 at 11:23:36PM -0700, Eli Friedman
wrote:
> Per subject, ARMovie/RPL demuxer; I think this
addresses all the
> review comments.

build system part OK, some nits below

> --- libavformat/rpl.c	(revision 0)
> +++ libavformat/rpl.c	(revision 0)
>  -0,0 +1,409 
> +
> +    // Movie name

lowercase, same below

> +    // Audio format id

ID

> --- libavformat/allformats.c	(revision 12559)
> +++ libavformat/allformats.c	(working copy)
>  -138,6 +138,7 
>      REGISTER_MUXDEMUX (RM, rm);
>      REGISTER_MUXDEMUX (ROQ, roq);
> +    REGISTER_DEMUXER  (RPL, rpl);
>      REGISTER_DEMUXER  (REDIR, redir);

I just fixed alphabetical order here so you will have to
refresh your
patch.

Diego
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Thu, Mar 27, 2008 at 11:23:36PM -0700, Eli Friedman
wrote:
> Per subject, ARMovie/RPL demuxer; I think this
addresses all the
> review comments.

[...]

> +    // 10000 is an arbitrary number; I can't imagine
anything
> +    // uses a higher fps.  Note that the limits are
> +    // selected so that the calculation of the
numerator
> +    // won't overflow.
> +    if (whole > 10000) {
> +        *error = -1;
> +        return result;
> +    }
> +    if (*endptr == '.') {
> +        line = endptr + 1;
> +        frac = strtoul(line, &endptr, 10);
> +        if (frac >= 0xFFFFFFFFUL) {
> +            *error = -1;
> +            return result;
> +        }
> +    }
> +    if (!whole && !frac) {
> +        *error = -1;
> +        return result;
> +    }
> +    den = 1;
> +    for (i = 0; i < endptr - line; i++)
> +        den *= 10;
> +    num = den * whole + frac;

for(c= get_byte(); c>='0' && c<='9'; c=
get_byte())
    num= 10*num + c - '0';
if(c == '.')
    c= get_byte();
for(; c>='0' && c<='9'; c= get_byte()){
    num= 10*num + c - '0';
    den*=10;
}
while(c != 'n' && !url_feof())
    c= get_byte()

similar can be used for read_int()


[...]
> +    // ARMovie
> +    error |= read_line(pb, line, sizeof(line));
> +
> +    // Movie name
> +    error |= read_line(pb, s->title,
sizeof(s->title));
> +
> +    // Date/copyright
> +    error |= read_line(pb, s->copyright,
sizeof(s->copyright));
> +
> +    // Author and other
> +    error |= read_line(pb, s->author,
sizeof(s->author));

following is prettier IMHO:

error |= read_line(pb, line        , sizeof(line)        );
// ARMovie
error |= read_line(pb, s->title    , sizeof(s->title) 
  ); // Movie name
error |= read_line(pb, s->copyright,
sizeof(s->copyright)); // Date/copyright
error |= read_line(pb, s->author   , sizeof(s->author)
  ); // Author and other


[...]
> +    } else {
> +        url_fseek(pb, index_entry->pos, SEEK_SET);
> +        ret = av_get_packet(pb, pkt,
index_entry->size);
> +        if (ret != index_entry->size) {
> +            av_free_packet(pkt);
> +            return AVERROR(EIO);
> +        }
> +

> +        if (stream->codec->codec_type ==
CODEC_TYPE_VIDEO) {

> +            // There should always be frames_per_chunk
frames in a chunk.
> +            pkt->duration =
rpl->frames_per_chunk;

There should be only 1 frame in each AVPacket



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

No human being will ever know the Truth, for even if they
happen to say it
by chance, they would not even known they had done so. --
Xenophanes

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Fri, Mar 28, 2008 at 6:42 AM, Michael Niedermayer
<michaelnigmx.at> wrote:
>  > +            // There should always be
frames_per_chunk frames in a chunk.
>  > +            pkt->duration =
rpl->frames_per_chunk;
>
>  There should be only 1 frame in each AVPacket

There's no possible way to enforce that correctly for an
unknown
codec; in general, the frame boundaries can be anywhere
within the
chunk.   Like the comment in the patch says, Escape 124
gets
special-cased, Escape 122 always has
rpl->frames_per_chunk == 1, and I
haven't coded the special-casing for Escape 130. (Taking a
look at the
description, I think the special-casing for Escape 130 is
the same as
for Escape 124, but I can't really test without a working
Escape 130
implementation.).

What exactly am I supposed to do here?

-Eli
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Fri, Mar 28, 2008 at 01:09:18PM -0700, Eli Friedman
wrote:
> On Fri, Mar 28, 2008 at 6:42 AM, Michael Niedermayer
<michaelnigmx.at> wrote:
> >  > +            // There should always be
frames_per_chunk frames in a chunk.
> >  > +            pkt->duration =
rpl->frames_per_chunk;
> >
> >  There should be only 1 frame in each AVPacket
> 
> There's no possible way to enforce that correctly for
an unknown
> codec; in general, the frame boundaries can be anywhere
within the
> chunk.   Like the comment in the patch says, Escape 124
gets
> special-cased, Escape 122 always has
rpl->frames_per_chunk == 1, and I
> haven't coded the special-casing for Escape 130.
(Taking a look at the
> description, I think the special-casing for Escape 130
is the same as
> for Escape 124, but I can't really test without a
working Escape 130
> implementation.).
> 
> What exactly am I supposed to do here?

Print a warning if frames_per_chunk>1 and the code doesnt
know how to
split it.

[...]

-- 
Michael     GnuPG fingerprint:
9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know
they do not
approve, and what they approve I do not know. -- Epicurus

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel