[FFmpeg-devel,3/9] avformat/s337m: Consider container bit resolution

Prepare the support of s337m in muxers other than raw (ex: wav).
For example, this forbids reading 16 bits DolbyE stream from a 24 bit wav file.
---
 libavformat/s337m.c | 20 ++++++++++++++------
 libavformat/s337m.h |  3 ++-
 2 files changed, 16 insertions(+), 7 deletions(-)

fre 2020-01-03 klockan 16:56 +0100 skrev Nicolas Gaullier:
> Prepare the support of s337m in muxers other than raw (ex: wav).
> For example, this forbids reading 16 bits DolbyE stream from a 24 bit wav file.
> ---
>  libavformat/s337m.c | 20 ++++++++++++++------
>  libavformat/s337m.h |  3 ++-
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/libavformat/s337m.c b/libavformat/s337m.c
> index eb26952834..5c8ab2649c 100644
> --- a/libavformat/s337m.c
> +++ b/libavformat/s337m.c
> @@ -34,7 +34,7 @@
>  
>  static int s337m_get_offset_and_codec(void *avc,
>                                        uint64_t state,
> -                                      int data_type, int data_size,
> +                                      int data_type, int data_size, int container_word_bits,
>                                        int *offset, enum AVCodecID *codec)
>  {
>      int word_bits;
> @@ -55,6 +55,11 @@ static int s337m_get_offset_and_codec(void *avc,
>              avpriv_report_missing_feature(avc, "Data type %#x in SMPTE 337M", data_type & 0x1F);
>          return AVERROR_PATCHWELCOME;
>      }
> +    if (container_word_bits && (container_word_bits+7)/8 != (word_bits+7)/8) {

Can it happen that word_bits is anything but 16 or 24 with a valid
stream? If not then I'd check container_word_bits == word_bits &&
(word_bits == 16 || word_bits == 24) or so

> +        if (avc)
> +            av_log(avc, AV_LOG_ERROR, "s337m: Container is %d bits vs. stream is %d bits\n", container_word_bits, word_bits);
> +        return AVERROR_INVALIDDATA;
> +    }
>  
>      if (codec)
>          *codec = AV_CODEC_ID_DOLBY_E;
> @@ -104,7 +109,7 @@ static int s337m_probe(const AVProbeData *p)
>              data_size = AV_RL24(buf + 3);
>          }
>  
> -        if (s337m_get_offset_and_codec(NULL, state, data_type, data_size, &offset, NULL))
> +        if (s337m_get_offset_and_codec(NULL, state, data_type, data_size, 0, &offset, NULL))
>              continue;
>  
>          i = IS_16LE_MARKER(state) ? 0 : IS_20LE_MARKER(state) ? 1 : 2;
> @@ -142,13 +147,16 @@ static void bswap_buf24(uint8_t *data, int size)
>          FFSWAP(uint8_t, data[0], data[2]);
>  }
>  
> -int ff_s337m_get_packet(AVIOContext *pb, AVPacket *pkt, int size, enum AVCodecID *codec, void *avc)
> +int ff_s337m_get_packet(AVIOContext *pb, AVPacket *pkt, int size, enum AVCodecID *codec, void *avc, int container_word_bits)
>  {
>      uint64_t state = 0;
>      int ret, data_type, data_size, offset;
>      int64_t pos, orig_pos = avio_tell(pb);
>  
> -    while (!IS_LE_MARKER(state)) {
> +    if (container_word_bits && container_word_bits != 16 && container_word_bits != 24)
> +        return AVERROR_INVALIDDATA;
> +    while ((container_word_bits == 24 || !IS_16LE_MARKER(state))
> +        && (container_word_bits == 16 || !IS_20LE_MARKER(state) && !IS_24LE_MARKER(state))) {

I'd rewrite this as while ((bits == 24 && (20LE || 24LE)) || (bits ==
20 && 16LE)), more readable

/Tomas

>> +    if (container_word_bits && (container_word_bits+7)/8 != 
>> + (word_bits+7)/8) {
>
>Can it happen that word_bits is anything but 16 or 24 with a valid stream? If not then I'd check container_word_bits == word_bits && (word_bits == 16 || word_bits == 24) or so
word_bits may be 20, and in that case container_word_bits must be 24 (this is the case in my fate test), so I think this is correct.

>> +    while ((container_word_bits == 24 || !IS_16LE_MARKER(state))
>> +        && (container_word_bits == 16 || !IS_20LE_MARKER(state) && 
>> + !IS_24LE_MARKER(state))) {
>
>I'd rewrite this as while ((bits == 24 && (20LE || 24LE)) || (bits ==20 && 16LE)), more readable
container_word_bits may be 0 for autodetect, this results in this expression...
I agree it is not that great for readability, but doing otherwise would require some additions and macros duplications, for example:
while ( !(!bits && LE) || !(bits == 24 && (20LE || 24LE)) || !(bits ==16 && 16LE))
Sounds heavy, not sure this is really better ?

Nicolas

mån 2020-01-13 klockan 14:02 +0000 skrev Gaullier Nicolas:
> > > +    if (container_word_bits && (container_word_bits+7)/8 != 
> > > + (word_bits+7)/8) {
> > 
> > Can it happen that word_bits is anything but 16 or 24 with a valid
> > stream? If not then I'd check container_word_bits == word_bits &&
> > (word_bits == 16 || word_bits == 24) or so
> word_bits may be 20, and in that case container_word_bits must be 24
> (this is the case in my fate test), so I think this is correct.

I think explicit checks for these three cases would be better instead
of rounding up. I wouldn't be surprised if there are files out there
that claim to be 18-bit or whatever..

> > > +    while ((container_word_bits == 24 || !IS_16LE_MARKER(state))
> > > +        && (container_word_bits == 16 || !IS_20LE_MARKER(state)
> > > && 
> > > + !IS_24LE_MARKER(state))) {
> > 
> > I'd rewrite this as while ((bits == 24 && (20LE || 24LE)) || (bits
> > ==20 && 16LE)), more readable
> container_word_bits may be 0 for autodetect, this results in this
> expression...
> I agree it is not that great for readability, but doing otherwise
> would require some additions and macros duplications, for example:
> while ( !(!bits && LE) || !(bits == 24 && (20LE || 24LE)) || !(bits
> ==16 && 16LE))
> Sounds heavy, not sure this is really better ?

Hrm, not sure tbh

/Tomas