[FFmpeg-devel] [PATCH] avformat: add NSP demuxer

[Carl Eugen Hoyos]

2017-12-04 21:36 GMT+01:00 Paul B Mahol <onemda at gmail.com>:
> On 12/4/17, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>> 2017-12-04 17:17 GMT+01:00 Paul B Mahol <onemda at gmail.com>:
>>> On 12/3/17, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>>>> 2017-12-01 17:26 GMT+01:00 Paul B Mahol <onemda at gmail.com>:
>>>>
>>>>> +static int nsp_read_header(AVFormatContext *s)
>>>>> +{
>>>>> +    int rate = 0, channels = 0;
>>>>> +    uint32_t chunk, size;
>>>>> +    AVStream *st;
>>>>> +    int64_t pos;
>>>>> +
>>>>> +    avio_skip(s->pb, 12);
>>>>
>>>> I believe you could set the duration here for the
>>>> supported interleaved stereo files.
>>>
>>> I prefer not.
>>
>> Isn't this what libavformat normally does?
>
> It auto calculates it from bitrate.

This will fail for multi-channel files and it is always a little
less accurate.

>>>>> +    st = avformat_new_stream(s, NULL);
>>>>> +    if (!st)
>>>>> +        return AVERROR(ENOMEM);
>>>>> +
>>>>> +    while (!avio_feof(s->pb)) {
>>>>> +        chunk = avio_rb32(s->pb);
>>>>> +        size  = avio_rl32(s->pb);
>>>>> +        pos   = avio_tell(s->pb);
>>>>> +
>>>>> +        if (chunk == MKBETAG('H', 'D', 'R', '8') ||
>>>>> +            chunk == MKBETAG('H', 'E', 'D', 'R')) {
>>>>> +            if (size < 32)
>>>>> +                return AVERROR_INVALIDDATA;
>>>>> +            avio_skip(s->pb, 20);
>>>>> +            rate = avio_rl32(s->pb);
>>>>
>>>>> +            avio_skip(s->pb, size - (avio_tell(s->pb) - pos));
>>>>
>>>> Why is this not "skip(pb, size - 32)" (or whatever the correct
>>>> constant is)?
>>>
>>> To be furure proof.
>>
>> To a specification change?
>
> No.

How can it be future-proof then?

>>>>> +        } else if (chunk == MKBETAG('N', 'O', 'T', 'E')) {
>>>>> +            char value[1024];
>>>>> +
>>>>> +            avio_get_str(s->pb, size, value, sizeof(value));
>>>>> +            av_dict_set(&s->metadata, "Note", value, 0);
>>>>
>>>> Shouldn't this be "comment"?
>>>
>>> No.
>>
>> Wouldn't this make the metadata compatible with other containers?
>
> Nope.

Is there really a container that understands metadata "Note": I did
not immediately find one.

>>>>> +            avio_skip(s->pb, 1);
>>>>
>>>> Should be something like "avio_skip(pb, size & 1);" according
>>>> to the description I found.
>>>
>>> Changed.
>>>
>>>>
>>>>> +        } else if (chunk == MKBETAG('S', 'D', 'A', 'B')) {
>>>>> +            channels = 2;
>>>>
>>>> If I read correctly, you can set STEREO here.
>>>
>>> I prefer not.
>>
>> Are the files not stereo?
>> (I am not sure I understand what the format is used for.)
>
> No, container does not store channel layout.

The specification implies (iirc) that SDAB contains a left and
right channel: Isn't this the definition of stereo?

>>>>> +            break;
>>>>
>>>>> +        } else if (chunk == MKBETAG('S', 'D', 'A', '_') ||
>>>>> +                   chunk == MKBETAG('S', 'D', '_', 'A') ||
>>>>> +                   chunk == MKBETAG('S', 'D', '_', '2') ||
>>>>> +                   chunk == MKBETAG('S', 'D', '_', '3') ||
>>>>> +                   chunk == MKBETAG('S', 'D', '_', '4') ||
>>>>> +                   chunk == MKBETAG('S', 'D', '_', '5') ||
>>>>> +                   chunk == MKBETAG('S', 'D', '_', '6') ||
>>>>> +                   chunk == MKBETAG('S', 'D', '_', '7') ||
>>>>> +                   chunk == MKBETAG('S', 'D', '_', '8')) {
>>>>
>>>> Iiuc, in these cases the file is not read correctly.
>>>> If this cannot be implemented easily, a warning should
>>>> be shown.
>>>
>>> I doubt so.
>>
>> Please correct me:
>> If the file is not SDAB, the channels are non-interleaved and
>> the chunks follow each other sequentially but are not read
>> by your current demuxer, no?
>
> Give me such files first!

I have never seen an nsp file, I only know of this format
thanks to you.
The only specification I found indicates that for >2 channels,
the sound data is not interleaved. Do you read the
specification differently?

Carl Eugen