Re: [FFmpeg-devel] [PATCH 3/9] avformat/s337m: Consider container bit resolution
mån 2020-01-13 klockan 14:02 + 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 3/9] avformat/s337m: Consider container bit resolution
>> +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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 3/9] avformat/s337m: Consider container bit resolution
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,
> , NULL))
> +if (s337m_get_offset_and_codec(NULL, state, data_type, data_size, 0,
> , 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 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(-)
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) {
+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,
, NULL))
+if (s337m_get_offset_and_codec(NULL, state, data_type, data_size, 0,
, 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))) {
state = (state << 8) | avio_r8(pb);
if (avio_feof(pb))
return AVERROR_EOF;
@@ -168,7 +176,7 @@ int ff_s337m_get_packet(AVIOContext *pb, AVPacket *pkt, int
size, enum AVCodecID
pos = avio_tell(pb);
-if ((ret = s337m_get_offset_and_codec(avc, state, data_type, data_size,
, codec)) < 0)
+if ((ret = s337m_get_offset_and_codec(avc, state, data_type, data_size,
container_word_bits, , codec)) < 0)
return ret;
if (ret = av_new_packet(pkt, FFMIN(offset, size - (pos - orig_pos))) < 0)
@@ -194,7 +202,7 @@ int ff_s337m_read_packet(AVFormatContext *s, AVPacket *pkt)
enum AVCodecID codec;
int ret;
-if ((ret = ff_s337m_get_packet(s->pb, pkt, avio_size(s->pb), , s)) <
0)
+if ((ret = ff_s337m_get_packet(s->pb, pkt, avio_size(s->pb), , s,
0)) < 0)
return ret;
if (!s->nb_streams) {
diff --git a/libavformat/s337m.h b/libavformat/s337m.h
index 86d2da6f57..d78ee8c501 100644
--- a/libavformat/s337m.h
+++ b/libavformat/s337m.h
@@ -30,9 +30,10 @@
* @param size Maximum IO read size available for reading at current position
* @param codec Returns AV_CODEC_ID_DOLBY_E
* @param avc For av_log
+ * @param container_word_bits 16,24, or 0 for autodetect
* @return = 0 on success (an error is raised if no s337m was found)
*/
-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);
int ff_s337m_read_packet(AVFormatContext *s, AVPacket *pkt);
#endif /* AVFORMAT_S337M_H */
--
2.14.1.windows.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
