[FFmpeg-devel] [PATCH 1/2] avcodec/dca_parser: Extend DTS core sync word

[foo86]

On Tue, Apr 28, 2015 at 07:27:06PM +0200, Michael Niedermayer wrote:
> On Tue, Apr 28, 2015 at 05:45:35PM +0300, foo86 wrote:
> > Check extended sync word for 16-bit LE and BE core streams to reduce 
> > probability of alias sync detection. Previously sync word extension was
> > checked only for 14-bit streams. 
> > 
> > This is sufficient to make the sample in ticket #4492 parse successfully.
> > The proper solution, however, would involve taking extension sub-stream
> > length into account by the parser.
> > ---
> >  libavcodec/dca_parser.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/dca_parser.c b/libavcodec/dca_parser.c
> > index 9797760..41658a5 100644
> > --- a/libavcodec/dca_parser.c
> > +++ b/libavcodec/dca_parser.c
> > @@ -38,7 +38,9 @@ typedef struct DCAParseContext {
> >  #define IS_MARKER(state, i, buf, buf_size) \
> >      ((state == DCA_SYNCWORD_CORE_14B_LE && (i < buf_size - 2) && (buf[i + 1] & 0xF0) == 0xF0 &&  buf[i + 2]         == 0x07) || \
> >       (state == DCA_SYNCWORD_CORE_14B_BE && (i < buf_size - 2) &&  buf[i + 1]         == 0x07 && (buf[i + 2] & 0xF0) == 0xF0) || \
> > -      state == DCA_SYNCWORD_CORE_LE || state == DCA_SYNCWORD_CORE_BE || state == DCA_SYNCWORD_SUBSTREAM)
> > +     (state == DCA_SYNCWORD_CORE_LE     && (i < buf_size - 2) && (buf[i + 2] & 0xFC) == 0xFC) || \
> > +     (state == DCA_SYNCWORD_CORE_BE     && (i < buf_size - 1) && (buf[i + 1] & 0xFC) == 0xFC) || \
> > +     state == DCA_SYNCWORD_SUBSTREAM)
> 
> i dont think this is safe, buf_size could be always 1, a parser
> could be feeded by 1 byte only in each call

You are right. I didn't realize existing check was unsafe before reusing
it.

> also testing with a few random dts files this patch causes many
> changes like:
> is this intended ?
> 
> tested with https://samples.mplayerhq.hu/A-codecs/DTS/dts/5.1%2024bit.dts
> 
> --- ref2.crc    2015-04-28 19:21:47.619285678 +0200
> +++ new2.crc    2015-04-28 19:21:44.187285606 +0200
> @@ -56,8 +56,7 @@
>  ,      960,     2012, 0x2583fa70
>  ,      960,     2012, 0x979ff3f3
>  ,      960,     2012, 0xbc32f58f
> -,      960,     2012, 0xbef8eb01
> -,      960,     2012, 0x9a37115b
> +,      960,     4024, 0xc153fc5c
>  ,      960,     2012, 0x5625f402
>  ,      960,     2012, 0xab29f377
>  ,      960,     2012, 0xc528e7ad
> @@ -312,8 +311,7 @@
>  ,      960,     2012, 0x334da9f1
>  ,      960,     2012, 0xbd47b990
>  ,      960,     2012, 0x4fa0cd82
> -,      960,     2012, 0x3bc2c8cb
> -,      960,     2012, 0xa68ccb2b
> +,      960,     4024, 0x5a479405
>  ,      960,     2012, 0x3c64bf12
>  ,      960,     2012, 0xb473bd06
>  ,      960,     2012, 0x9b1ac192
> 
> [...]

This is not intended, likely the consequence of IS_MARKER check not
working correctly at the buffer boundary. I'll try to come up with a
better way to fix sync detection.