| Message ID | 20171201162643.11222-1-onemda@gmail.com |
|---|---|
| State | New |
| Headers | show |
2017-12-01 17:26 GMT+01:00 Paul B Mahol <onemda@gmail.com>: > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > libavformat/Makefile | 1 + > libavformat/allformats.c | 1 + > libavformat/nspdec.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 103 insertions(+) > create mode 100644 libavformat/nspdec.c > > diff --git a/libavformat/Makefile b/libavformat/Makefile > index 4bffdf2205..734b703862 100644 > --- a/libavformat/Makefile > +++ b/libavformat/Makefile > @@ -331,6 +331,7 @@ OBJS-$(CONFIG_MXF_MUXER) += mxfenc.o mxf.o audiointerleave.o > OBJS-$(CONFIG_MXG_DEMUXER) += mxg.o > OBJS-$(CONFIG_NC_DEMUXER) += ncdec.o > OBJS-$(CONFIG_NISTSPHERE_DEMUXER) += nistspheredec.o pcm.o > +OBJS-$(CONFIG_NSP_DEMUXER) += nspdec.o > OBJS-$(CONFIG_NSV_DEMUXER) += nsvdec.o > OBJS-$(CONFIG_NULL_MUXER) += nullenc.o > OBJS-$(CONFIG_NUT_DEMUXER) += nutdec.o nut.o isom.o > diff --git a/libavformat/allformats.c b/libavformat/allformats.c > index 9213af9301..6a9b9883c9 100644 > --- a/libavformat/allformats.c > +++ b/libavformat/allformats.c > @@ -224,6 +224,7 @@ static void register_all(void) > REGISTER_DEMUXER (MXG, mxg); > REGISTER_DEMUXER (NC, nc); > REGISTER_DEMUXER (NISTSPHERE, nistsphere); > + REGISTER_DEMUXER (NSP, nsp); > REGISTER_DEMUXER (NSV, nsv); > REGISTER_MUXER (NULL, null); > REGISTER_MUXDEMUX(NUT, nut); > diff --git a/libavformat/nspdec.c b/libavformat/nspdec.c > new file mode 100644 > index 0000000000..d2ff779732 > --- /dev/null > +++ b/libavformat/nspdec.c > @@ -0,0 +1,101 @@ > +/* > + * NSP demuxer > + * Copyright (c) 2017 Paul B Mahol > + * > + * This file is part of FFmpeg. > + * > + * FFmpeg is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * FFmpeg is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with FFmpeg; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > + */ > + > +#include "libavutil/avstring.h" > +#include "libavutil/intreadwrite.h" > +#include "avformat.h" > +#include "internal.h" > +#include "pcm.h" > + > +static int nsp_probe(AVProbeData *p) > +{ > + if (AV_RB32(p->buf) == AV_RB32("FORM") && > + AV_RB32(p->buf + 4) == AV_RB32("DS16")) > + return AVPROBE_SCORE_MAX; > + return 0; > +} > + > +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); > + st = avformat_new_stream(s, NULL); > + if (!st) > + return AVERROR(ENOMEM); > + > + while (!avio_feof(s->pb)) { > + chunk = avio_rb32(s->pb); Not codec_tag? > + size = avio_rl32(s->pb); > + pos = avio_tell(s->pb); > + > + if (chunk == MKBETAG('H', 'D', 'R', '8') || > + chunk == MKBETAG('H', 'E', 'D', 'R')) { It's your code but I suspect a switch() makes this more readable. Carl Eugen
On 12/3/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-12-01 17:26 GMT+01:00 Paul B Mahol <onemda@gmail.com>: >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> --- >> libavformat/Makefile | 1 + >> libavformat/allformats.c | 1 + >> libavformat/nspdec.c | 101 >> +++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 103 insertions(+) >> create mode 100644 libavformat/nspdec.c >> >> diff --git a/libavformat/Makefile b/libavformat/Makefile >> index 4bffdf2205..734b703862 100644 >> --- a/libavformat/Makefile >> +++ b/libavformat/Makefile >> @@ -331,6 +331,7 @@ OBJS-$(CONFIG_MXF_MUXER) += mxfenc.o >> mxf.o audiointerleave.o >> OBJS-$(CONFIG_MXG_DEMUXER) += mxg.o >> OBJS-$(CONFIG_NC_DEMUXER) += ncdec.o >> OBJS-$(CONFIG_NISTSPHERE_DEMUXER) += nistspheredec.o pcm.o >> +OBJS-$(CONFIG_NSP_DEMUXER) += nspdec.o >> OBJS-$(CONFIG_NSV_DEMUXER) += nsvdec.o >> OBJS-$(CONFIG_NULL_MUXER) += nullenc.o >> OBJS-$(CONFIG_NUT_DEMUXER) += nutdec.o nut.o isom.o >> diff --git a/libavformat/allformats.c b/libavformat/allformats.c >> index 9213af9301..6a9b9883c9 100644 >> --- a/libavformat/allformats.c >> +++ b/libavformat/allformats.c >> @@ -224,6 +224,7 @@ static void register_all(void) >> REGISTER_DEMUXER (MXG, mxg); >> REGISTER_DEMUXER (NC, nc); >> REGISTER_DEMUXER (NISTSPHERE, nistsphere); >> + REGISTER_DEMUXER (NSP, nsp); >> REGISTER_DEMUXER (NSV, nsv); >> REGISTER_MUXER (NULL, null); >> REGISTER_MUXDEMUX(NUT, nut); >> diff --git a/libavformat/nspdec.c b/libavformat/nspdec.c >> new file mode 100644 >> index 0000000000..d2ff779732 >> --- /dev/null >> +++ b/libavformat/nspdec.c >> @@ -0,0 +1,101 @@ >> +/* >> + * NSP demuxer >> + * Copyright (c) 2017 Paul B Mahol >> + * >> + * This file is part of FFmpeg. >> + * >> + * FFmpeg is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Lesser General Public >> + * License as published by the Free Software Foundation; either >> + * version 2.1 of the License, or (at your option) any later version. >> + * >> + * FFmpeg is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * Lesser General Public License for more details. >> + * >> + * You should have received a copy of the GNU Lesser General Public >> + * License along with FFmpeg; if not, write to the Free Software >> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >> 02110-1301 USA >> + */ >> + >> +#include "libavutil/avstring.h" >> +#include "libavutil/intreadwrite.h" >> +#include "avformat.h" >> +#include "internal.h" >> +#include "pcm.h" >> + >> +static int nsp_probe(AVProbeData *p) >> +{ >> + if (AV_RB32(p->buf) == AV_RB32("FORM") && >> + AV_RB32(p->buf + 4) == AV_RB32("DS16")) >> + return AVPROBE_SCORE_MAX; >> + return 0; >> +} >> + >> +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); >> + st = avformat_new_stream(s, NULL); >> + if (!st) >> + return AVERROR(ENOMEM); >> + >> + while (!avio_feof(s->pb)) { >> + chunk = avio_rb32(s->pb); > > Not codec_tag? No. > >> + size = avio_rl32(s->pb); >> + pos = avio_tell(s->pb); >> + >> + if (chunk == MKBETAG('H', 'D', 'R', '8') || >> + chunk == MKBETAG('H', 'E', 'D', 'R')) { > > It's your code but I suspect a switch() makes this > more readable. Changed.
2017-12-01 17:26 GMT+01:00 Paul B Mahol <onemda@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. > + 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)? > + } 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"? > + avio_skip(s->pb, 1); Should be something like "avio_skip(pb, size & 1);" according to the description I found. > + } else if (chunk == MKBETAG('S', 'D', 'A', 'B')) { > + channels = 2; If I read correctly, you can set STEREO here. > + 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. Thank you, Carl Eugen
On 12/3/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-12-01 17:26 GMT+01:00 Paul B Mahol <onemda@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. > >> + 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. > >> + } 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. > >> + 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. > >> + 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.
2017-12-04 17:17 GMT+01:00 Paul B Mahol <onemda@gmail.com>: > On 12/3/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> 2017-12-01 17:26 GMT+01:00 Paul B Mahol <onemda@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
On 12/4/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-12-04 17:17 GMT+01:00 Paul B Mahol <onemda@gmail.com>: >> On 12/3/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>> 2017-12-01 17:26 GMT+01:00 Paul B Mahol <onemda@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. > >>>> + 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. > >>>> + } 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. > >>>> + 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. > >>>> + 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!
2017-12-04 21:36 GMT+01:00 Paul B Mahol <onemda@gmail.com>: > On 12/4/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> 2017-12-04 17:17 GMT+01:00 Paul B Mahol <onemda@gmail.com>: >>> On 12/3/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>> 2017-12-01 17:26 GMT+01:00 Paul B Mahol <onemda@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
On 12/4/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-12-04 21:36 GMT+01:00 Paul B Mahol <onemda@gmail.com>: >> On 12/4/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>> 2017-12-04 17:17 GMT+01:00 Paul B Mahol <onemda@gmail.com>: >>>> On 12/3/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>> 2017-12-01 17:26 GMT+01:00 Paul B Mahol <onemda@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.
2017-12-04 21:54 GMT+01:00 Paul B Mahol <onemda@gmail.com>: > On 12/4/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> 2017-12-04 21:36 GMT+01:00 Paul B Mahol <onemda@gmail.com>: >>> On 12/4/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>> 2017-12-04 17:17 GMT+01:00 Paul B Mahol <onemda@gmail.com>: >>>>> On 12/3/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>>> 2017-12-01 17:26 GMT+01:00 Paul B Mahol <onemda@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
On 12/4/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-12-04 21:54 GMT+01:00 Paul B Mahol <onemda@gmail.com>: >> On 12/4/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>> 2017-12-04 21:36 GMT+01:00 Paul B Mahol <onemda@gmail.com>: >>>> On 12/4/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>> 2017-12-04 17:17 GMT+01:00 Paul B Mahol <onemda@gmail.com>: >>>>>> On 12/3/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>>>> 2017-12-01 17:26 GMT+01:00 Paul B Mahol <onemda@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.
diff --git a/libavformat/Makefile b/libavformat/Makefile index 4bffdf2205..734b703862 100644 --- a/libavformat/Makefile +++ b/libavformat/Makefile @@ -331,6 +331,7 @@ OBJS-$(CONFIG_MXF_MUXER) += mxfenc.o mxf.o audiointerleave.o OBJS-$(CONFIG_MXG_DEMUXER) += mxg.o OBJS-$(CONFIG_NC_DEMUXER) += ncdec.o OBJS-$(CONFIG_NISTSPHERE_DEMUXER) += nistspheredec.o pcm.o +OBJS-$(CONFIG_NSP_DEMUXER) += nspdec.o OBJS-$(CONFIG_NSV_DEMUXER) += nsvdec.o OBJS-$(CONFIG_NULL_MUXER) += nullenc.o OBJS-$(CONFIG_NUT_DEMUXER) += nutdec.o nut.o isom.o diff --git a/libavformat/allformats.c b/libavformat/allformats.c index 9213af9301..6a9b9883c9 100644 --- a/libavformat/allformats.c +++ b/libavformat/allformats.c @@ -224,6 +224,7 @@ static void register_all(void) REGISTER_DEMUXER (MXG, mxg); REGISTER_DEMUXER (NC, nc); REGISTER_DEMUXER (NISTSPHERE, nistsphere); + REGISTER_DEMUXER (NSP, nsp); REGISTER_DEMUXER (NSV, nsv); REGISTER_MUXER (NULL, null); REGISTER_MUXDEMUX(NUT, nut); diff --git a/libavformat/nspdec.c b/libavformat/nspdec.c new file mode 100644 index 0000000000..d2ff779732 --- /dev/null +++ b/libavformat/nspdec.c @@ -0,0 +1,101 @@ +/* + * NSP demuxer + * Copyright (c) 2017 Paul B Mahol + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include "libavutil/avstring.h" +#include "libavutil/intreadwrite.h" +#include "avformat.h" +#include "internal.h" +#include "pcm.h" + +static int nsp_probe(AVProbeData *p) +{ + if (AV_RB32(p->buf) == AV_RB32("FORM") && + AV_RB32(p->buf + 4) == AV_RB32("DS16")) + return AVPROBE_SCORE_MAX; + return 0; +} + +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); + 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)); + } 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); + avio_skip(s->pb, 1); + } else if (chunk == MKBETAG('S', 'D', 'A', 'B')) { + channels = 2; + 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')) { + channels = 1; + break; + } + } + + st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO; + st->codecpar->channels = channels; + st->codecpar->sample_rate = rate; + st->codecpar->codec_id = AV_CODEC_ID_PCM_S16LE; + st->codecpar->block_align = 2 * channels; + + return 0; +} + +AVInputFormat ff_nsp_demuxer = { + .name = "nsp", + .long_name = NULL_IF_CONFIG_SMALL("Computerized Speech Lab NSP"), + .read_probe = nsp_probe, + .read_header = nsp_read_header, + .read_packet = ff_pcm_read_packet, + .read_seek = ff_pcm_read_seek, + .extensions = "nsp", + .flags = AVFMT_GENERIC_INDEX, +};
Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavformat/Makefile | 1 + libavformat/allformats.c | 1 + libavformat/nspdec.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+) create mode 100644 libavformat/nspdec.c