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

Michael Niedermayer michaelni
Sat Nov 29 02:44:08 CET 2008


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


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


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

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

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


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

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- 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/20081129/b7bc3c99/attachment.pgp>



More information about the ffmpeg-devel mailing list