[FFmpeg-devel] [PATCH] MPEG sequeunce display extension parsing off by 2 error

[Daniel Kristjansson]

On Tue, 2010-06-29 at 13:16 +0200, Michael Niedermayer wrote:
> > 13818-2: 1995(E), p33. 
> 
> mine says 1995 too and it doesnt say draft (which doesnt mean that it
> isnt one ...), can you confirm that yours is not a draft?

Mine doesn't say draft, but I don't really know if it is. I bought a
copy at some point, but the copy I'm looking at doesn't have my name
embedded in it like most of the ones I've bought.

> more precissely mines says:
> 6.2.2.4     Sequence display extension
> 
> |sequence_display_extension() {                |No. of  |Mnemonic|
> |                                              |bits    |        |
> | extension_start_code_identifier              |4       |uimsbf  |
> | video_format                                 |3       |uimsbf  |
> | colour_description                           |1       |uimsbf  |
> | if ( colour_description ) {                  |        |        |
> |  colour_primaries                            |8       |uimsbf  |
> |  transfer_characteristics                    |8       |uimsbf  |
> |  matrix_coefficients                         |8       |uimsbf  |
> | }                                            |        |        |
> | display_horizontal_size                      |14      |uimsbf  |
> | marker_bit                                   |1       |bslbf   |
> | display_vertical_size                        |14      |uimsbf  |
> | next_start_code()                            |        |        |
> |}                                             |        |        |

Reading your copy it looks like those 3 bits at the end are totally
unaccounted for. Things are byte aligned before display_horizontal_size
and the next_start_code will be byte aligned, but we've only got
29 bits in between. Those three bits are the steganography bits. ;)

> > They are not actually marker bits, they are
> > undefined bits, so they do not have to be set like the bit between
> > the horizontal and vertical size.
> 
> that means your patch is wrong. (more precissly the comment is)

True, I'll generate a new patch. But it's your call on whether to
just remove the "skip_bits(&s->gb, 1); // marker" or replace it with 
"skip_bits(&s->gb, 3); // padding". Given the ambiguity, my preference
is just to drop the last skip_bits completely unless there is some
efficiency reason to be byte aligned when we finish parsing this.

-- Daniel