[FFmpeg-devel] [PATCH 2/4] mov: check for positive sample->size

[Andreas Cadhalpun]

On 25.05.2015 18:31, Michael Niedermayer wrote:
> On Mon, May 25, 2015 at 05:25:18PM +0200, Andreas Cadhalpun wrote:
>> On 24.05.2015 19:23, Michael Niedermayer wrote:
>>> ISO/IEC 14496-12:2012(E) says the field is unsigned so it cannot be
>>> negative
>>>
>>> 8.7.4.2   Syntax
>>> aligned(8) class SampleToChunkBox
>>>     extends FullBox("stsc", version = 0, 0) {
>>>     unsigned int(32) entry_count;
>>>     for (i=1; i <= entry_count; i++) {
>>>         unsigned int(32) first_chunk;
>>>         unsigned int(32) samples_per_chunk;
>>>         unsigned int(32) sample_description_index;
>>>     }
>>> }
>>
>> OK, but then the types of the members of MOVStsc and likely also MOVStts
>> are incorrectly int. The first attached patch changes that.
> 
> MOVStts needs to stay signed, its used for ctts and that is signed
> per spec
> aligned(8) class CompositionOffsetBox
>    extends FullBox("ctts", version = 0, 0) {
>    unsigned int(32) entry_count;
>       int i;
>    if (version==0) {
>       for (i=0; i < entry_count; i++) {
>          unsigned int(32) sample_count;
>          unsigned int(32) sample_offset;
>       }
>    }
>    else if (version == 1) {
>       for (i=0; i < entry_count; i++) {
>          unsigned int(32) sample_count;
>          signed   int(32) sample_offset;
>       }
>    }
> }
> 
> also above is just the ISO side, theres also a quicktime spec
> that is seperate and while similar enough so our demuxer supports both
> they are seperate file formats
> https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/qtff.pdf

That one says about ctts/stts:
ctts:
 * sampleCount
    A 32-bit unsigned integer that provides the number of consecutive
    samples with the calculated composition offset in the field.
 * compositionOffset
    A 32-bit signed integer indicating the value of the calculated
    compositionOffset.

stts:
 * Sample count
    A 32-bit integer that specifies the number of consecutive samples
    that have the same duration.
 * Sample duration
    A 32-bit integer that specifies the duration of each sample.

So it can be signed or unsigned.

For stsc it doesn't even mention the sign:
 * First chunk
    The first chunk number using this table entry.
 * Samples per chunk
    The number of samples in each chunk.
 * Sample description ID
    The identification number associated with the sample description
    for the sample. For details on sample description atoms, see Sample
    Description Atoms (page 99).

So if you don't like the first patch changing the types, consider it dropped.
It shouldn't change the behavior anyway.

I'm more interested in the second patch validating the
bytes_per_frame/samples_per_frame combination.

The quicktime spec says about these:
* Samples per packet (sc->samples_per_frame)
    The number of uncompressed frames generated by a compressed frame
    (an uncompressed frame is one sample from each channel). This is
    also the frame duration, expressed in the media's timescale, where
    the timescale is equal to the sample rate. For uncompressed formats,
    this field is always 1.
 * Bytes per packet (ignored)
    For uncompressed audio, the number of bytes in a sample for a single
    channel. This replaces the older sampleSize field, which is set to 16.
    This value is calculated by dividing the frame size by the number of
    channels. The same calculation is performed to calculate the value of
    this field for compressed audio, but the result of the calculation is
    not generally meaningful for compressed audio.
 * Bytes per frame (sc->bytes_per_frame)
    The number of bytes in a frame: for uncompressed audio, an
    uncompressed frame; for compressed audio, a compressed frame. This
    can be calculated by multiplying the bytes per packet field by the
    number of channels.
 * Bytes per sample (ignored)
    The size of an uncompressed sample in bytes. This is set to 1 for
    8-bit audio, 2 for all other cases, even if the sample size is
    greater than 2 bytes.
[...]
If these fields are not used, they are set to 0. File readers only need
to check to see if samplesPerPacket is 0.

This suggests that if the field is 0, it should not be used.
That's the check my last patch implemented, so I think it should be fine.

Best regards,
Andreas