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

Carl Eugen Hoyos ceffmpeg at gmail.com
Mon Dec 4 22:59:50 EET 2017


2017-12-04 21:54 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 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.

But the nsp container does store the duration.

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

The description of the chunk says that the chunk contains
the comment.
Apart from that, shouldn't we try (hard) to make metadata
compatible between formats?

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

You mean wavesurfer does not support >two channels
while the file format does?
How does that invalidate my suggestion to print a
warning in that case?

Carl Eugen


More information about the ffmpeg-devel mailing list