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

[Michael Niedermayer]

On Tue, Dec 02, 2008 at 01:39:03PM +1100, Anuradha Suraparaju wrote:
> Hi, 
> 
> Sorry for the delayed response. 

noone beats my delayed responses, i stil have mails from last year i should
awnser ...


> My replies are inline.
[...]
> > > 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.

what happens with a 
display order I B B I B B P ...
coded order   I I B B P B B ...
?
the way i understand your explanation is that the first 2 I frames would
have has_b_frames=0 and then when the first B is encountered it is set to 1
this doesnt seem correct. has_b_frames should be 1 from the begin ideally
In that respect i wonder if it wouldnt be more correct to just always set
has_b_frames=1, but then i dont know dirac, its really a question about
how a 1-in 1-out decoder would work

i mean if it returned I frames with a delay of 1 then has_b_frames=1 would
be correct. If it didnt then i wonder what it would do with
I,I,B
immedeatly decoding&returning the I frames would cause a
problem once it encounters the B frame.


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

not really, i mean, once you found a correct 'BBCD' you dont have to search
for 'BBCD' anymore you can just use the parse offset stored in the bitstream
(and just verify that they are correct by checking if a 'BBCD' occurs at the
expected next point)
And before the first 'BBCD' is found or if the previous check fails simply
starting above procss at the next 'BBCD' should work fine.
Theres of course no need to do it like that, i just feel it could
be done simpler than your implementation and possibly use ff_combine_frame()
though i may be missing some reason why ff_combine_frame() cannot be used
easily ...

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

There will always be a question for which you do not know the correct awnser.
-------------- 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/20081219/2c9e73f1/attachment.pgp>