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

[Carl Eugen Hoyos]

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?

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

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

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

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

Carl Eugen