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

[Anuradha Suraparaju]

Hi,
On Sun, 2008-06-15 at 02:14 +0200, Michael Niedermayer wrote:
> 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.

Fixed. Changed this condition to
if ( next_pu_offset <= 0 || parse_ctx->buf_size < next_pu_offset)
    return NULL;

A null offset for a non-End of Sequence packet is also not valid. 
> 
> 
> > +
> > +    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.

Fixed.
 
> 
> 
> >      }
> >  
> > +    /* 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

Fixed.

> 
> and the other patch in this email looks ok
> 
> [...]

Regards,
Anuradha
-------------- next part --------------
A non-text attachment was scrubbed...
Name: issue453_fix_pts_bug_libschroedingerdec_svn_13775.diff
Type: text/x-patch
Size: 3918 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080616/6f0611b2/attachment.bin>