[FFmpeg-devel] [PATCH] Patches to fix issue453 in libdiracschroedinger

[Michael Niedermayer]

On Mon, Jun 09, 2008 at 02:26:41PM +1000, Anuradha Suraparaju wrote:
> Hi,
> 
> 
> On Tue, 2008-06-03 at 23:48 +0200, Michael Niedermayer wrote:
> > On Sun, Jun 01, 2008 at 06:03:12PM +1000, Anuradha Suraparaju wrote:
> > > Hi, 
> > > 
> > > I have addressed most of the issues mentioned in your email in the new
> > > patches. 
> > [...]
> > 
> > > > [...]
> > > > > Index: libavcodec/dirac_parser.c
> > > > > ===================================================================
> > > > > --- libavcodec/dirac_parser.c	(revision 13233)
> > > > > +++ libavcodec/dirac_parser.c	(working copy)
> > > > > @@ -62,16 +62,12 @@
> > > > >      ParseContext *pc = s->priv_data;
> > > > >      int next;
> > > > >  
> > > > > -    if (s->flags & PARSER_FLAG_COMPLETE_FRAMES) {
> > > > > -        next = buf_size;
> > > > > -    }else{
> > > > > -        next = find_frame_end(pc, buf, buf_size);
> > > > > +    next = find_frame_end(pc, buf, buf_size);
> > > > >  
> > > > > -        if (ff_combine_frame(pc, next, &buf, &buf_size) < 0) {
> > > > > -            *poutbuf = NULL;
> > > > > -            *poutbuf_size = 0;
> > > > > -            return buf_size;
> > > > > -        }
> > > > > +    if (ff_combine_frame(pc, next, &buf, &buf_size) < 0) {
> > > > > +        *poutbuf = NULL;
> > > > > +        *poutbuf_size = 0;
> > > > > +        return buf_size;
> > > > >      }
> > > > >  
> > > > >      *poutbuf = buf;
> > > > 
> > > > The current code in dirac_parser.c looks correct to me, this change should
> > > > not be needed.
> > > > PARSER_FLAG_COMPLETE_FRAMES is supposed to mean "complete" in the ffmpeg
> > > > sense.
> > > > 
> > > 
> > > As I mentioned in another email, libschroedinger requires that packet
> > > sent to it be Dirac byte stream parse units. If a packet contains more
> > > than one parse-unit the second gets ignored. Hence the change I made is
> > > required.
> > 
> > What ffmpeg calls complete frames is what a parser should output, hence
> > if PARSER_FLAG_COMPLETE_FRAMES is set there is no spliting to do for a
> > parser.
> > 
> > If i understand the current code correctly the parser does not behave that
> > way and the decoder would not work if it did.
> > If true -vcodec copy will likely not work with dirac currently
> > 
> 
> I've left dirac_parser.c unchanged from the svn version and have modfied
> libschroedingerdec.c to to sub-parse "complete" ffmpeg frames into
> individual parse unit. I am attaching two patches to this email
> 
> issue453_fix_pts_bug_common_libdirac_libschroedinger_svn_13724.diff -
> files common to libdirac and libschroedinger
> fix_pts_bug_libdirac_svn_13594.diff
> issue453_fix_pts_bug_libschroedingerdec_svn_13724.diff
> 
> The earlier patches fix_pts_bug_libdirac_svn_13594.diff (libdiracenc.c)
> and issue453_fix_pts_bug_libschroedinger_svn_13594.diff
> libschroedingerenc.c) work as is with svn revision 17324.

[...]


> +static SchroBuffer* FfmpegFindNextSchroParseUnit (FfmpegSchroParseUnitContext *parse_ctx)
> +{
> +    SchroBuffer *enc_buf = NULL;
> +    int next_pu_offset = 0;
> +    unsigned char *in_buf;
> +
> +    if (parse_ctx->buf_size < 13 ||
> +        parse_ctx->buf[0] != 'B' ||
> +        parse_ctx->buf[1] != 'B' ||
> +        parse_ctx->buf[2] != 'C' ||
> +        parse_ctx->buf[3] != 'D')
> +        return NULL;
> +
> +    next_pu_offset = (parse_ctx->buf[5] << 24) +
> +                     (parse_ctx->buf[6] << 16) +
> +                     (parse_ctx->buf[7] <<  8) +
> +                      parse_ctx->buf[8];
> +
> +    if (next_pu_offset == 0 && 
> +        SCHRO_PARSE_CODE_IS_END_OF_SEQUENCE(parse_ctx->buf[4]))
> +        next_pu_offset = 13;
> +
> +    if (parse_ctx->buf_size < next_pu_offset)
> +        return NULL;

both buf_size and next_pu_offset are signed and it seems next_pu_offset could
easily be <0 given the wrong input. If that happened this condition would be
false even though it surely should be true.


> +
> +    in_buf = av_malloc(next_pu_offset);
> +    memcpy (in_buf, parse_ctx->buf, next_pu_offset);
> +    enc_buf = schro_buffer_new_with_data (in_buf, next_pu_offset);
> +    enc_buf->free = libschroedinger_decode_buffer_free;
> +    enc_buf->priv = in_buf;
> +
> +    parse_ctx->buf += next_pu_offset;
> +    parse_ctx->buf_size -= next_pu_offset;
> +
> +    return enc_buf;
> +}
> +
>  /**
>  * Returns FFmpeg chroma format.
>  */
> @@ -164,30 +218,33 @@
>      SchroFrame* frame;
>      int state;
>      int go = 1;
> +    int outer = 1;
> +    FfmpegSchroParseUnitContext parse_ctx;
>  
>      *data_size = 0;
>  
> -    if (buf_size>0) {
> -        unsigned char *in_buf = av_malloc(buf_size);
> -        memcpy (in_buf, buf, buf_size);
> -        enc_buf = schro_buffer_new_with_data (in_buf, buf_size);
> -        enc_buf->free = libschroedinger_decode_buffer_free;
> -        enc_buf->priv = in_buf;
> -        /* Push buffer into decoder. */
> -        state = schro_decoder_push (decoder, enc_buf);
> -        if (state == SCHRO_DECODER_FIRST_ACCESS_UNIT)
> -            libschroedinger_handle_first_access_unit(avccontext);
> -    } else {
> +    FfmpegSchroParseContextInit (&parse_ctx, buf, buf_size);
> +    if (buf_size == 0) {

> -        if (!p_schro_params->eos_signalled) {
> +           if (!p_schro_params->eos_signalled) {
> -            state = schro_decoder_push_end_of_stream(decoder);
> +               state = schro_decoder_push_end_of_stream(decoder);
> -            p_schro_params->eos_signalled = 1;
> +               p_schro_params->eos_signalled = 1;
> -        }
> +           }

cosmetics, and the indention is even less correct afterwards.


>      }
>  
> +    /* Loop through all the individual parse units in the input buffer */
> +    do {
> +        if ((enc_buf = FfmpegFindNextSchroParseUnit(&parse_ctx))) {
> +               /* Push buffer into decoder. */
> +               state = schro_decoder_push (decoder, enc_buf);
> +               if (state == SCHRO_DECODER_FIRST_ACCESS_UNIT)
> +                     libschroedinger_handle_first_access_unit(avccontext);
> +            go = 1;
> +        }
> +        else
> +            outer = 0;

> +        format = p_schro_params->format;
> -    format = p_schro_params->format;
> -
> -    while (go) {
>          /* Parse data and process result. */
> +        while (go) {

cosmetics

and the other patch in this email looks ok

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

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- 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/20080615/ef9a5e6a/attachment.pgp>