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

Anuradha Suraparaju anuradha
Mon May 26 02:26:59 CEST 2008


On Fri, 2008-05-23 at 22:15 +0200, Michael Niedermayer wrote:
> On Thu, May 22, 2008 at 12:26:16PM +1000, Anuradha Suraparaju wrote:
> > Hi,
> > 
> > I've included 2 patches to fix the bug reported in issue453. More
> > details of the bug can be found in
> > 
> > http://roundup.mplayerhq.hu/roundup/ffmpeg/issue453
> > 
> > The same issue applies to libdirac too. So I included a patch for
> > libdirac as well.
> > 
> > Non-frame data is either prepended (sequence headers) or appended (end
> > of sequence info to the last frame) to frame data to ensure that the
> > codec outputs frame data in every packet and the pts is monotonous. So a
> > packet output by the encoder will contain one encoded frame and header
> > or sequence end info when applicable. 
> > 
> > 1. issue453_fix_pts_bug_common_libdirac_libschroedinger.diff
> > 
> >    Contains patched files common to libdirac and libschroedinger.
> > 
> >    libavcodec/libdirac_libschro.c{.h}
> > 
> >    libavcodec/dirac_parser.c - Since a packet can now contain more than 
> >    one Dirac parse unit, a complete packet will still need to be parsed
> >    to extract a single Dirac parse unit.
> > 
> >    libavformat/riff.c   - Add a fourcc code for CODEC_ID_DIRAC to enable
> >    wrapping and playback of Dirac in AVI.
> > 
> > 2. issue453_fix_pts_bug_libschroedinger.diff
> > 
> >    Contains patch to libschroedingerenc.c to fix the non-monotone pts 
> >    bug
> > 
> > 3. fix_pts_bug_libdirac.diff
> > 
> >    Contains patch to libdiracenc.c to ensure that pts is monotnous.
> [...]
> > @@ -104,6 +106,7 @@
> >      FfmpegDiracSchroQueueElement *top = queue->p_head;
> >  
> >      if (top != NULL) {
> > +        --queue->size;
> >          void *data = top->data;
> >          queue->p_head = queue->p_head->next;
> >          av_freep (&top);
> 
> mixes declaration and statement thus breaks gcc 2.95 support.
> 

OK. Will fix this.

> 
> > @@ -112,3 +115,8 @@
> >  
> >      return NULL;
> >  }
> > +
> > +int32_t ff_dirac_schro_queue_size (FfmpegDiracSchroQueue *queue)
> > +{
> > +    return queue->size;
> > +}
> 
> Senseless wraper function.
> 

OK. Will use queue->size directly in code instead of having a function.

> 
> > Index: libavcodec/libdirac_libschro.h
> > ===================================================================
> > --- libavcodec/libdirac_libschro.h	(revision 13233)
> > +++ libavcodec/libdirac_libschro.h	(working copy)
> > @@ -80,6 +80,8 @@
> >      FfmpegDiracSchroQueueElement *p_head;
> >      /** Pointer to tail of queue */
> >      FfmpegDiracSchroQueueElement *p_tail;
> 
> > +    /** Queue size*/
> > +    int32_t size;
> 
> I dont think this needs to be exactly 32bit, a int should do.
> 

OK

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

The Schroedinger library expects to be given one parse unit at a time.
Since we are bundling the sequence headers and other non-frame data with
frame data to keep the pts sane, when PARSER_FLAG_COMPLETE_FRAMES flag
is set, we end up sending more than one parse unit to the library which
results in incorrect decoding. This happens when Dirac bytestream is
wrapped in AVI. Hence the change to ignore the
PARSER_FLAG_COMPLETE_FRAMES flag when parsing.

> 
> [...]
> > @@ -252,9 +258,10 @@
> >      SchroEncoder *encoder = p_schro_params->encoder;
> >      struct FfmpegDiracSchroEncodedFrame* p_frame_output = NULL;
> >      int go = 1;
> > -    SchroBuffer *enc_buf;
> > +    SchroBuffer *schro_buf;
> >      int presentation_frame;
> >      int parse_code;
> 
> cosmetic changes must be in seperate patches from functional changes.
> 

OK.

> 
> [...]
> 
> > +            if (p_schro_params->enc_buf_size == 0)
> > +                p_schro_params->enc_buf = av_malloc(schro_buf->length);
> > +            else {
> > +                p_schro_params->enc_buf = av_realloc (
> > +                             p_schro_params->enc_buf,
> > +                             p_schro_params->enc_buf_size + schro_buf->length
> > +                             );
> > +            }
> 
> This is the same as just
> p_schro_params->enc_buf = av_realloc (
>         p_schro_params->enc_buf,
>         p_schro_params->enc_buf_size + schro_buf->length
>         );
> 
> 
> 
> > +            memcpy(p_schro_params->enc_buf+p_schro_params->enc_buf_size,
> > +                   schro_buf->data, schro_buf->length);
> > +            p_schro_params->enc_buf_size += schro_buf->length;
> > +
> > +
> 
> > +            if (state == SCHRO_STATE_END_OF_STREAM) {
> > +                p_schro_params->eos_pulled = 1;
> > +                   go = 0;
> > +            }
> 
> the
>     indention
>         looks
> a little odd
> 

OK. Will fix the indentation.

> 
> [...]
> >                  p_frame_output->key_frame = 1;
> >              }
> > -
> >              /* Parse the coded frame number from the bitstream. Bytes 14
> >               * through 17 represesent the frame number. */
> 
> cosmetic

OK. 

> 
> 
> > -            if (SCHRO_PARSE_CODE_IS_PICTURE(parse_code))
> > -            {
> > -                assert (enc_buf->length >= 17);
> 
> > -                p_frame_output->frame_num = (enc_buf->data[13] << 24) +
> > -                                            (enc_buf->data[14] << 16) +
> > -                                            (enc_buf->data[15] <<  8) +
> > -                                             enc_buf->data[16];
> > -            }
> > +            p_frame_output->frame_num = (schro_buf->data[13] << 24) +
> > +                                        (schro_buf->data[14] << 16) +
> > +                                        (schro_buf->data[15] <<  8) +
> > +                                         schro_buf->data[16];
> 
> reindention counts as cosmetic as well
> so yes the if&assert should be removed and the indention then fixed in a
> seperate patch, this keeps svnlog and patches more human readable.
> 

I want to make sure that I understood this correctly. So I remove the if
& assert in one patch and submit it. Then I fix the indentation and
other cosmetic changes mentions and submit a second patch. Is this
correct?

> 
> [...]


Regards,
Anuradha





More information about the ffmpeg-devel mailing list