[FFmpeg-devel] [PATCH]Fix for issue694. Dirac A/V sync loss

Anuradha Suraparaju anuradha
Tue Dec 2 03:39:03 CET 2008


Hi, 

Sorry for the delayed response. My replies are inline.

On Sat, 2008-11-29 at 02:44 +0100, Michael Niedermayer wrote:
> On Tue, Nov 18, 2008 at 11:34:02AM +1100, Anuradha Suraparaju wrote:
> > Hi,
> > 
> > On Fri, 2008-11-07 at 04:41 -0500, David Conrad wrote:
> > [...]
> > > 
> > > > +    pu->next_pu_offset = (start[5] << 24) +
> > > > +                         (start[6] << 16) +
> > > > +                         (start[7] << 8)  +
> > > > +                         start[8];
> > > 
> > > AV_RL32(start+5) is more readable imo.
> > 
> > AV_RL32 is used when the MSB is last and not first. So replaced the
> > above with AV_RB32 instead.
> > 
> > Also removed code related to unnecessary parsing of the sequence header
> > since the decoder is invoked in av_find_stream_info when parsing a raw
> > Dirac bitstream.
> > 
> > A modified patch is attached to this email.
> > 
> > Regards,
> > Anuradha 
> > 
> 
> > Index: libavcodec/libdiracdec.c
> > ===================================================================
> > --- libavcodec/libdiracdec.c	(revision 15870)
> > +++ libavcodec/libdiracdec.c	(working copy)
> > @@ -88,10 +88,12 @@
> >  
> >      *data_size = 0;
> >  
> > -    if (buf_size>0)
> > +    if (buf_size>0) {
> >          /* set data to decode into buffer */
> >          dirac_buffer (p_dirac_params->p_decoder, buf, buf+buf_size);
> > -
> > +        if ((buf[4] &0x08) == 0x08 && (buf[4] & 0x03))
> > +            avccontext->has_b_frames = 1;
> > +    }
> >      while (1) {
> >           /* parse data and process result */
> >          DecoderState state = dirac_parse (p_dirac_params->p_decoder);
> 
> Just to make sure this code does what you intend it to do. (has_b_frames is
> poorly named ...) and i dont know dirac well enough to understand what the
> checked bits represent exactly
> 
> has_b_frames == 1 means that a decoder would have a 1 frame reordering buffer
> (like mpeg1/2 with IPB frames where IP are delayed while B are not)
> has_b_frames=0 means that a decoder would not have any frame delay, that also
> implicates that there is no frame reordering. (mpeg2 low delay is an example
> of this) in mpeg1/2 no reordering also implicates no b frames
> has_b_frames=1 does not require that there are B frames
> 
> also has_b_frames is mainly used for filling in missing timestamps
> 

Dirac, the specification, has a flexible GOP structure. So the frame
re-ordering can be anything. This said, the current implementations of
Dirac (dirac-research and Schroedinger) use a frame-reordering on 1.
Intra and backward predicted frames can be delayed but bi-directional
frames are not.  So the mpeg1/2 logic for has_b_frames holds. In Dirac
it is not possible to tell whether a frame is a backward predicted frame
(similar to 'P' frame) or a bi-directional frame (similar to 'B' frame)
easily without processing its reference frames. So I set has_b_frames to
1 if I come across any predicted frame (buf[4] &0x08) == 0x08 - mean the
parse unit is a picture, and (buf[4] & 0x03 checks the number of
references) . So an I-frame only sequence will set has_b_frames to 0 but
a sequence having only P or P&B frames will set it to 1 since
has_b_frames=1 does not require that there be any 'B' frames in the
sequence.

> 
> [...]
> > +    if ( next == -1) {
> > +        /* Found a possible frame start but not a frame end */
> > +        void *new_buffer = av_realloc (pc->buffer,
> > +                              pc->buffer_size + (*buf_size - pc->sync_offset));
> > +        pc->buffer = new_buffer;
> > +        memcpy (pc->buffer+pc->buffer_size, (*buf + pc->sync_offset),
> > +                *buf_size - pc->sync_offset);
> > +        pc->buffer_size += *buf_size - pc->sync_offset;
> > +        return -1;
> > +    } else {
> > +        /* Found a possible frame start and a  possible frame end */
> > +        DiracParseUnit pu1, pu;
> > +        void *new_buffer = av_realloc (pc->buffer, pc->buffer_size + next);
> > +        pc->buffer = new_buffer;
> > +        memcpy (pc->buffer + pc->buffer_size, *buf, next);
> 
> I would prefer if av_realloc() and or memcpy() could be avoided more often,
> if av_realloc() cant be avoided easily, then av_fast_realloc() could be
> considered as its quite a bit faster at least last time ive seen benchmarks
> of it
> 
> 

Will change it to av_fast_realloc in the next update to the patch.

> [...]
> >  static int dirac_parse(AVCodecParserContext *s, AVCodecContext *avctx,
> >                         const uint8_t **poutbuf, int *poutbuf_size,
> >                         const uint8_t *buf, int buf_size)
> >  {
> > -    ParseContext *pc = s->priv_data;
> > +    DiracParseContext *pc = s->priv_data;
> >      int next;
> >  
> > +    *poutbuf = NULL;
> > +    *poutbuf_size = 0;
> > +
> >      if (s->flags & PARSER_FLAG_COMPLETE_FRAMES) {
> >          next = buf_size;
> > -    }else{
> > +        *poutbuf = buf;
> > +        *poutbuf_size = buf_size;
> > +        /* Assume that data has been packetized into an encapsulation unit */
> > +    } else {
> >          next = find_frame_end(pc, buf, buf_size);
> > +        if (!pc->is_synced && next == -1) {
> > +            /* Did not find a frame start yet. So throw away the entire
> > +               buffer */
> > +            *poutbuf = NULL;
> > +            *poutbuf_size = 0;
> > +            return buf_size;
> > +        }
> >  
> > -        if (ff_combine_frame(pc, next, &buf, &buf_size) < 0) {
> > +        if (dirac_combine_frame(s, avctx, next, &buf, &buf_size) < 0) {
> >              *poutbuf = NULL;
> >              *poutbuf_size = 0;
> 
> i think poutbuf / poutbuf_size are alraedy 0 here and above
> 

Will fix this.

> And could you explain why you do not use ff_combine_frame() but instead
> implement your own?
> I mean AC3 also can use ff_combine_frame() (see aac_ac3_parser.c/ac3_parser.c)
> and it does also merge some frames (EAC3 stuff) and is a
> "startcode" + framesize format that i belive is somewhat similar to dirac
> 

Dirac is a bit trickier. Since we use Arithmetic coding for entropy
coding the sync sequence 'BBCD' can occur in the entropy data and can
trigger a false start of frame. We can't go by frame size (here the next
parse offset) alone because of this. So we need to use the parse offsets
of the current frame and the next frame to validate the current frame.
The previous parse offset of the next frame must match the next parse
offset of the current frame. If they don't match the current frame gets
thrown out. So I decided to implement a Dirac specific combine frame.
Hope this explains it.
 
> But then i surely must admit our ac3 parser had and possibly still has bugs
> thats besides some miscompiltions with gcc when optimizations are turned on ...
> 
> 
> [...]

Regards,
Anuradha

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





More information about the ffmpeg-devel mailing list