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

[Paul B Mahol]

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

Where?

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

OK, Changed.

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

OK, will add warning for others.