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

Paul B Mahol onemda at gmail.com
Mon Dec 4 22:54:28 EET 2017


On 12/4/17, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
> 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.

I disagree. If container does not store duration it should not be
hacked in demuxer.

>
>>>>>> +    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?

For case of reading header for multi channels files.

>
>>>>>> +        } 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.

If it is comment it would have different chunk name.

>
>>>>>> +            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?

Nope.

>
>>>>>> +            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?

For 1 channels wavesurfer generates SDA_ files.

For >2 channels it creates 2 channels SDAB files.


More information about the ffmpeg-devel mailing list