[FFmpeg-devel] Add Ogg Skeleton 4 support

[Michael Niedermayer]

Hi

First, please keep ffmpeg-dev in CC

On Tue, Sep 27, 2011 at 03:50:11AM +0200, j at v2v.cc wrote:
> Add Ogg Skeleton 4 support to the ogg demuxer. For more info on OggIndex
> and Ogg Skeleton see https://wiki.xiph.org/Ogg_Skeleton_4
> 
> Addressing Diego's comments.

> If the vlc is in libav already it should be used of cause.

I dont think the ffmpeg codebase contains this and libav doesnt
contain anything we dont so i dont think libav contains such function.

Functions doing similar things:
ffio_read_varlen()
ff_mp4_read_descr_len()
bs_get_v()
get_base128()
read_arbitary()
klv_decode_ber_length()

the libav people could clean this up for us and give these consistent
names ...

also

" The variable byte encoded integers are encoded using 7 bits per byte to store the integer's bits, and the high bit is set in the last byte used to encode the integer. The bits and bytes are in little endian byte order. For example, the integer 7843, or 0001 1110 1010 0011 in binary, would be stored as two bytes: 0xBD 0x23, or 1011 1101 0010 0011 in binary. "

the specification clearly says little endian byte order and if it
was that way get_base128() could be used.
but then the specification contains a example that is in big endian
order.
Also the example has the last high bit clear while the text says set

I think you should report these issue, it would be nice if xiph would
not use a different encoding in each of their standards



[...]

>  oggparseskeleton.c |   75 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 69 insertions(+), 6 deletions(-)
> b292bf27c5083fe736660a861c10d7052be332a9  0001-Add-Ogg-Skeleton-4-support-to-the-ogg-demuxer.patch
> diff --git a/libavformat/oggparseskeleton.c b/libavformat/oggparseskeleton.c
> index ceb7c69..a2a31ba 100644
> --- a/libavformat/oggparseskeleton.c
> +++ b/libavformat/oggparseskeleton.c
> @@ -22,15 +22,38 @@
>  #include "avformat.h"
>  #include "oggdec.h"
>  
> +static int skeleton_read_vlc (uint8_t ** data, int * size, int64_t * result)

the uint8_t should be marked as const
the return value should possibly be int64_t
and the actual value read should be checked to be within the allowed
range somewhere. It may be possible to do this inside this function
Note that without checks this can overflow and become negative



[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110927/15198fd1/attachment.asc>