[FFmpeg-devel,v3,3/6] avformat/s337m: Make available as subdemuxer

---
 libavformat/s337m.c | 64 ++++++++++++++++++++++++++++++++++++++++++++---------
 libavformat/s337m.h | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+), 11 deletions(-)
 create mode 100644 libavformat/s337m.h

tis 2019-08-06 klockan 17:08 +0200 skrev Nicolas Gaullier:
> +int avpriv_s337m_probe_stream(void *avc, AVIOContext *pb, AVStream **st, int size)
> +{
> +    if ( size >= S337M_MIN_OFFSET &&
> +        ((*st)->codecpar->codec_id == AV_CODEC_ID_PCM_S16LE || (*st)->codecpar->codec_id == AV_CODEC_ID_PCM_S24LE) &&
> +               (*st)->codecpar->channels == 2) {
> +        uint8_t *buf;
> +        int64_t data_offset;
> +        int pos = 0;
> +
> +        size = FFMIN(size, S337M_MAX_OFFSET);
> +        if (!(buf = av_mallocz(size))) {
> +            return AVERROR(ENOMEM);
> +        }
> +        data_offset = avio_tell(pb);
> +        if (avio_read(pb, buf, size) != size) {
> +            av_freep(&buf);
> +            return AVERROR(EIO);
> +        }
> +        avio_seek(pb, data_offset, SEEK_SET);
> +
> +        while (pos < size - 9 && !buf[pos]) {
> +            pos++;
> +        }
> +        if (pos < size - 9 && pos >= S337M_PROBE_GUARDBAND_MIN_BYTES) {

I think this 9 should be an 11 or 12..

> +            uint64_t state;
> +            int data_type = -1, data_size, offset;
> +
> +            if ((*st)->codecpar->bits_per_coded_sample == 16) {
> +                state = AV_RB32(buf + pos);
> +                if (IS_16LE_MARKER(state)) {
> +                    data_type = AV_RL16(buf + pos + 4);
> +                    data_size = AV_RL16(buf + pos + 6);
> +                }
> +            } else if ((*st)->codecpar->bits_per_coded_sample == 24) {
> +                state = AV_RB48(buf + pos);
> +                if (IS_20LE_MARKER(state) || IS_24LE_MARKER(state)) {
> +                    data_type = AV_RL24(buf + pos + 6);
> +                    data_size = AV_RL24(buf + pos + 9);

.. because of this read

> +                }
> +            }
> +            if (data_type >= 0 && !s337m_get_offset_and_codec(avc, state, data_type, data_size, &offset, &(*st)->codecpar->codec_id)) {
> +                double s337m_phase = pos * 4 / (*st)->codecpar->bits_per_coded_sample / (*st)->codecpar->sample_rate;

This isn't quite what I meant by turning it into an integer test :)
this will likely just be rounded to zero. I meant that things could
likely be rearranged so there's no divisions, so the probing isn't
subject to the vaguaries of float rounding

> +#define DOLBY_E_PHASE_MIN       0.000610
> +#define DOLBY_E_PHASE_MAX       0.001050

Where do these phase values come from? Is there a spec somewhere?

I notice the ratios DOLBY_E_PHASE_MAX/DOLBY_E_PHASE_MIN and
S337M_MAX_OFFSET/S337M_MIN_OFFSET are within 9% of eachother, which
doesn't feel like a coincidence

/Tomas

>> +        if (pos < size - 9 && pos >= S337M_PROBE_GUARDBAND_MIN_BYTES) 

>I think this 9 should be an 11 or 12..

Indeed, thank you, my mistake.

>This isn't quite what I meant by turning it into an integer test :) this will likely just be rounded to zero. I meant that things could likely be rearranged so there's no divisions, so the probing isn't subject to the vaguaries of float >rounding

On one side, dealing with a macro that contains a double (S337M_PHASE_PROBE_MIN) makes it impossible at the end to avoid a test involving float rounding (in my understanding, but I may be wrong, and I remember there are some helpers in ffmpeg to make it easier to avoid float rounding), or maybe I should have limited the digits and simply worked with an integer number of milliseconds, but that sounded a little bit overkill, together with the fact that the default value is still 0.0000 so all of this is only in case a developer wants to change the #define.
On the other side, I don't think it was such a good idea to specify this in seconds as for the dolby_e min/max. This value is just for assurance that there will be no wrong probing, and I thought that just byte count was more appropriate... and that makes it an integer test at the end :)
So I replaced : 
  if (pos < size - 9 && 
  (s337m_phase = (double)pos * 4 / (*st)->codecpar->bits_per_coded_sample / (*st)->codecpar->sample_rate) >= S337M_PHASE_PROBE_MIN) {
With just simply :
  if (pos >= S337M_PROBE_GUARDBAND_MIN_BYTES)
And thus replaced S337M_PHASE_PROBE_MIN=0.000 by S337M_PROBE_GUARDBAND_MIN_BYTES=0.
At the end, I don't see any cons to do that, and it is far most simplest.

> +#define DOLBY_E_PHASE_MIN       0.000610

> +#define DOLBY_E_PHASE_MAX       0.001050

>Where do these phase values come from? Is there a spec somewhere?

https://www.dolby.com/us/en/technologies/dolby-e-line-position.pdf

Nicolas

Am Di., 6. Aug. 2019 um 17:08 Uhr schrieb Nicolas Gaullier
<nicolas.gaullier@arkena.com>:
>
> ---
>  libavformat/s337m.c | 64 ++++++++++++++++++++++++++++++++++++++++++++---------
>  libavformat/s337m.h | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 117 insertions(+), 11 deletions(-)
>  create mode 100644 libavformat/s337m.h

> diff --git a/libavformat/s337m.h b/libavformat/s337m.h
> new file mode 100644
> index 0000000000..0f21a23a30
> --- /dev/null
> +++ b/libavformat/s337m.h
> @@ -0,0 +1,64 @@
> +/*
> + * Copyright (C) 2017 foo86
> + *
> + * 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
> + */
> +
> +#ifndef AVFORMAT_S337M_H
> +#define AVFORMAT_S337M_H
> +
> +#define MARKER_16LE         0x72F81F4E
> +#define MARKER_20LE         0x20876FF0E154
> +#define MARKER_24LE         0x72F8961F4EA5
> +
> +#define IS_16LE_MARKER(state)   ((state & 0xFFFFFFFF) == MARKER_16LE)
> +#define IS_20LE_MARKER(state)   ((state & 0xF0FFFFF0FFFF) == MARKER_20LE)
> +#define IS_24LE_MARKER(state)   ((state & 0xFFFFFFFFFFFF) == MARKER_24LE)
> +#define IS_LE_MARKER(state)     (IS_16LE_MARKER(state) || IS_20LE_MARKER(state) || IS_24LE_MARKER(state))
> +
> +#define S337M_MIN_OFFSET 1601*4
> +#define S337M_MAX_OFFSET 2002*6
> +
> +#define S337M_PROBE_GUARDBAND_MIN_BYTES     0
> +#define DOLBY_E_PHASE_MIN       0.000610
> +#define DOLBY_E_PHASE_MAX       0.001050
> +
> +/**
> + * Detect s337m packets in a PCM_S16LE/S24LE stereo stream
> + * If found, the codec of the stream is updated
> + * Requires a single sample with enough (S337M_PROBE_GUARDBAND_MIN_BYTES) and clean (set to zero) guard band
> + * @param avc For av_log
> + * @param pb Associated IO context
> + * @param st Streams
> + * @param size Maximum IO read size available for probing at current position
> + * @return = 0 if no error encountered (even if no s337m was found)
> + */

> +int avpriv_s337m_probe_stream(void *avc, AVIOContext *pb, AVStream **st, int size);

Sorry for not commenting earlier:
This is to the best of my knowledge not acceptable (to have a public
function for a
particular format), instead create a new demuxer that supports this raw format.

Carl Eugen

tor 2019-08-08 klockan 10:28 +0000 skrev Gaullier Nicolas:
> > > +        if (pos < size - 9 && pos >=
> > > S337M_PROBE_GUARDBAND_MIN_BYTES) 
> > I think this 9 should be an 11 or 12..
> Indeed, thank you, my mistake.
> 
> > This isn't quite what I meant by turning it into an integer test :)
> > this will likely just be rounded to zero. I meant that things could
> > likely be rearranged so there's no divisions, so the probing isn't
> > subject to the vaguaries of float >rounding
> On one side, dealing with a macro that contains a double
> (S337M_PHASE_PROBE_MIN) makes it impossible at the end to avoid a
> test involving float rounding (in my understanding, but I may be
> wrong, and I remember there are some helpers in ffmpeg to make it
> easier to avoid float rounding), or maybe I should have limited the
> digits and simply worked with an integer number of milliseconds, but
> that sounded a little bit overkill, together with the fact that the
> default value is still 0.0000 so all of this is only in case a
> developer wants to change the #define.
> On the other side, I don't think it was such a good idea to specify
> this in seconds as for the dolby_e min/max. This value is just for
> assurance that there will be no wrong probing, and I thought that
> just byte count was more appropriate... and that makes it an integer
> test at the end :)
> So I replaced : 
>   if (pos < size - 9 && 
>   (s337m_phase = (double)pos * 4 / (*st)->codecpar-
> >bits_per_coded_sample / (*st)->codecpar->sample_rate) >=
> S337M_PHASE_PROBE_MIN) {
> With just simply :
>   if (pos >= S337M_PROBE_GUARDBAND_MIN_BYTES)
> And thus replaced S337M_PHASE_PROBE_MIN=0.000 by
> S337M_PROBE_GUARDBAND_MIN_BYTES=0.
> At the end, I don't see any cons to do that, and it is far most
> simplest.

Ah, so it's time and not phase? Renaming the #defines to make that
clearer would be nice. Other than that, this isn't a huge issue

/Tomas

>Ah, so it's time and not phase? Renaming the #defines to make that clearer would be nice. Other than that, this isn't a huge issue

I assume the start of the packet to be synched with the start of "a" video frame, I think there is no use case where pcm and video are not synced, at least when dolby E is concerned.
So, DOLBY_E_PHASE_MIN/MAX are phase and well named I think.
Concerning S337M_PHASE_PROBE_MIN that I renamed S337M_PROBE_GUARDBAND_MIN_BYTES, this is still a phase, a kind of safe guard phase, to make even more unlikely to have a wrong detection. But the question is to determine if this phase should be specified in milliseconds or just in raw bytes (or another man could say in number of samples whether there are 16 or 24 bits). There may be other approaches, but if we consider strictly the risk of statistical match, it is related to the number of raw bytes, this is why I choosed raw bytes at the end and thus the name S337M_PROBE_GUARDBAND_MIN_BYTES looks good to me.
Furthermore, at the end, this S337M_PROBE_GUARDBAND_MIN_BYTES is zero for now (so whatever the unit), and in my experience, this is generally required as there are some programs which indeed have a zero-phase Dolby E (and BTW it appears some softwares doesn't like it). Taking the worst case scenario (16 bits), the sync code takes 4 bytes, so the raw probability is only 1/2^32 (and even less taking into account that usually broadcast programs starts with a little bit of silence). So anyway, maybe I took too much care on this, but at least here this is explicit in the code, I think.

Nicolas

>> +int avpriv_s337m_probe_stream(void *avc, AVIOContext *pb, AVStream **st, int size);

>Sorry for not commenting earlier:

You're welcome, a few days is not that much, particularly in the middle of the summer!
>This is to the best of my knowledge not acceptable (to have a public function for a particular format), instead create a new demuxer that supports this raw format.

S337m is a very particular format though: potentially, it may be probed within any pcm stream, I don't see anything comparable in any other format.
The problem is, the developer will use a standard demuxer or device to open the streams (for example, a decklink, an mxf etc.), and then decide if he wants to process some of the streams as regular pcm or as dolby E to be decoded. I may be wrong, but I don't see how the same workflow could be obtained by creating a new separate demuxer.
Nicolas

Am Do., 8. Aug. 2019 um 14:18 Uhr schrieb Gaullier Nicolas
<nicolas.gaullier@arkena.com>:
>
> >> +int avpriv_s337m_probe_stream(void *avc, AVIOContext *pb, AVStream **st, int size);

> > This is to the best of my knowledge not acceptable (to have a public function for a particular
> > format), instead create a new demuxer that supports this raw format.
>
> S337m is a very particular format though:

Yes (like others)

> potentially, it may be probed within any pcm stream, I don't see anything comparable in any other format.

Look at spdif in wav.

> The problem is, the developer will use a standard demuxer or device to open the streams (for example, a
> decklink, an mxf etc.), and then decide if he wants to process some of the streams as regular pcm or as
> dolby E to be decoded. I may be wrong, but I don't see how the same workflow could be obtained by
> creating a new separate demuxer.

This will work without any issue (just as it does for dca in wav).

Carl Eugen

>Look at spdif in wav.

Indeed, I missed that, spdif and s337m are very similar technically, and it means I have to rework my patch for the integration of s337m in wavdec, but I still do not catch why another demuxer should be created : spdifdec.c is actually quite similar to the existing s337m.c
Perhaps you find unacceptable only the use of the avpriv_ prefix instead of ff_ ? I understand, but in a previous cover letter, I mentioned that the idea was to make this available to build a filter that would decode s337m in a filter graph; but yes, maybe I should stay with ff_ for now and migrate to avpriv_ later when submitting the new filter, that would make sense, moreover the prototype may be changed too depending on how the implementation of the filter turns to be (and I have no experience in writing a filter for ffmpeg for now, so...).

In my opinion, here are the most relevant differences between spdif and s337m, it is a matter of use cases :
- s337m may be split in two mono streams and may require an "audio merge" to be decodable, which suggests the use of a filter graph and a dedicated filter to decode s337/dolby_e (so the use of public methods for a dedicated format, indeed)
- dolby E is a codec one either don't want to see (ie. pass through) or one want to get rid off/decode to uncompressed/pcm, whereas ac3 for example is a very popular codec one usually wants to stream copy: so an audio filter to decode s337/dolbyE seems interesting whereas an audio filter to decode spdif/ac3 would be absolutely useless
- s337m may be frame wrapped (ex: mxf) (probing method differs)

At the end, in my current understanding, here is what I should correct :
- make the integration of s337m in wavdec similar to that of spdif
- replace the avpriv_* by ff_* ... for now

Anyway, I will wait your feedback and until we completely agree before going anywhere further.

Nicolas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".