[FFmpeg-devel] [PATCH 0/2]

[John-Paul Stewart]

Hi folks.  Let me preface this by saying that I'm a first-time
contributor, so please bear with me while I figure out proper
procedures.  (I've been reading the list for a while to try to
familiarize myself with how things are done.)

A recent project to convert some old videos in SGI .movie format
revealed that libavformat is mis-reading some of the metadata about
audio within those files.  Specifically, the value the library was using
for the number of audio channels is actually the sample size (in bytes)
and the code was skipping over the actual number of audio channels.  All
of the existing examples at samples.ffmpeg.org/sgi are either 16 bit
stereo or silent.  There were no single channel examples and no 8 bit
examples.

I have created four clips with SGI's original 'moviemaker' utility (so
they are guaranteed to be correctly formatted) to reverse engineer those
two pieces of metadata since the format is largely undocumented.  They
are under 1MB each and available at:
    http://www.personalprojects.net/ffmpeg/mono8bit.movie
    http://www.personalprojects.net/ffmpeg/mono16bit.movie
    http://www.personalprojects.net/ffmpeg/stereo8bit.movie
    http://www.personalprojects.net/ffmpeg/stereo16bit.movie
They'll only be available for a few weeks;  I don't want to have to host
them forever.  There are also corresponding text files (change .movie to
.txt in the URLs) with a bit of a description of what each one is and
how it is mis-interpreted.

There are three things in the patches that I need guidance on:

1) I added blank lines in a couple of places (before or after my
changes) to separate the processing of different pieces of data from
each other or from calls to avio_skip().  To me it is more readable that
way, but I realize it may not be appropriate to add those blank lines.

2) In patch 2/2 (last changed line) the original code was dividing by 
channels * 2 which I changed to channels * bytes_per_sample on the
assumption that the original "2" was due to all samples being treated as
2 bytes (16 bits).  But I'm not really clear on that hard-coded "2" so I
may be wrong.

3) Patch 1/2 adds a call to avio_skip() which is then removed in patch 
2/2.  This could be avoided by making it all one patch.  I'm not sure 
whether it is better to fix the handling of the number of channels in 
one patch and add support for sample size in the second, or to do it all 
in one patch to avoid adding and removing that call to avio_skip().

If anyone has feedback on those (or any other part of the patch set),
let me know.