[FFmpeg-devel] [PATCH v2 1/2] avcodec: Fix warnings with signed/unsigned compare in bitstream.h

[FFmpeg-devel] [PATCH v2 1/2] avcodec: Fix warnings with signed/unsigned compare in bitstream.h

[Devin Heitmueller]

When including the header in decklink_enc.cpp it would be fed
through the C++ compiler rather than the C compiler, which has
more strict warnings when comparing signed/unsigned values.

Make the local variables unsigned to match the arguments they are
being passed for those functions.

Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
---
 libavcodec/bytestream.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/libavcodec/bytestream.h b/libavcodec/bytestream.h
index d0033f14f3..67080604b9 100644
--- a/libavcodec/bytestream.h
+++ b/libavcodec/bytestream.h
@@ -180,7 +180,7 @@ static av_always_inline void bytestream2_skipu(GetByteContext *g,
 static av_always_inline void bytestream2_skip_p(PutByteContext *p,
                                                 unsigned int size)
 {
-    int size2;
+    unsigned int size2;
     if (p->eof)
         return;
     size2 = FFMIN(p->buffer_end - p->buffer, size);
@@ -268,7 +268,7 @@ static av_always_inline unsigned int bytestream2_get_buffer(GetByteContext *g,
                                                             uint8_t *dst,
                                                             unsigned int size)
 {
-    int size2 = FFMIN(g->buffer_end - g->buffer, size);
+    unsigned int size2 = FFMIN(g->buffer_end - g->buffer, size);
     memcpy(dst, g->buffer, size2);
     g->buffer += size2;
     return size2;
@@ -287,7 +287,7 @@ static av_always_inline unsigned int bytestream2_put_buffer(PutByteContext *p,
                                                             const uint8_t *src,
                                                             unsigned int size)
 {
-    int size2;
+    unsigned int size2;
     if (p->eof)
         return 0;
     size2 = FFMIN(p->buffer_end - p->buffer, size);
@@ -311,7 +311,7 @@ static av_always_inline void bytestream2_set_buffer(PutByteContext *p,
                                                     const uint8_t c,
                                                     unsigned int size)
 {
-    int size2;
+    unsigned int size2;
     if (p->eof)
         return;
     size2 = FFMIN(p->buffer_end - p->buffer, size);
@@ -348,7 +348,7 @@ static av_always_inline unsigned int bytestream2_copy_buffer(PutByteContext *p,
                                                              GetByteContext *g,
                                                              unsigned int size)
 {
-    int size2;
+    unsigned int size2;
 
     if (p->eof)
         return 0;
-- 
2.35.1.655.ga68dfadae5

_______________________________________________
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".

[FFmpeg-devel] [PATCH v2 2/2] decklink: Add support for compressed AC-3 output over SDI

[Devin Heitmueller]

Extend the decklink output to include support for compressed AC-3,
encapsulated using the SMPTE ST 377:2015 standard.

This functionality can be exercised by using the "copy" codec when
the input audio stream is AC-3.  For example:

./ffmpeg -i ~/foo.ts -codec:a copy -f decklink 'UltraStudio Mini Monitor'

Note that the default behavior continues to be to do PCM output,
which means without specifying the copy codec a stream containing
AC-3 will be decoded and downmixed to stereo audio before output.

Thanks to Marton Balint for providing feedback.

Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
---
 libavdevice/decklink_enc.cpp | 90 ++++++++++++++++++++++++++++++------
 1 file changed, 75 insertions(+), 15 deletions(-)

diff --git a/libavdevice/decklink_enc.cpp b/libavdevice/decklink_enc.cpp
index 8d423f6b6e..8d80f00247 100644
--- a/libavdevice/decklink_enc.cpp
+++ b/libavdevice/decklink_enc.cpp
@@ -32,6 +32,7 @@ extern "C" {
 
 extern "C" {
 #include "libavformat/avformat.h"
+#include "libavcodec/bytestream.h"
 #include "libavutil/internal.h"
 #include "libavutil/imgutils.h"
 #include "avdevice.h"
@@ -243,19 +244,32 @@ static int decklink_setup_audio(AVFormatContext *avctx, AVStream *st)
         av_log(avctx, AV_LOG_ERROR, "Only one audio stream is supported!\n");
         return -1;
     }
-    if (c->sample_rate != 48000) {
-        av_log(avctx, AV_LOG_ERROR, "Unsupported sample rate!"
-               " Only 48kHz is supported.\n");
-        return -1;
-    }
-    if (c->ch_layout.nb_channels != 2 && c->ch_layout.nb_channels != 8 && c->ch_layout.nb_channels != 16) {
-        av_log(avctx, AV_LOG_ERROR, "Unsupported number of channels!"
-               " Only 2, 8 or 16 channels are supported.\n");
+
+    if (c->codec_id == AV_CODEC_ID_AC3) {
+        /* Regardless of the number of channels in the codec, we're only
+           using 2 SDI audio channels at 48000Hz */
+        ctx->channels = 2;
+    } else if (c->codec_id == AV_CODEC_ID_PCM_S16LE) {
+        if (c->sample_rate != 48000) {
+            av_log(avctx, AV_LOG_ERROR, "Unsupported sample rate!"
+                   " Only 48kHz is supported.\n");
+            return -1;
+        }
+        if (c->ch_layout.nb_channels != 2 && c->ch_layout.nb_channels != 8 && c->ch_layout.nb_channels != 16) {
+            av_log(avctx, AV_LOG_ERROR, "Unsupported number of channels!"
+                   " Only 2, 8 or 16 channels are supported.\n");
+            return -1;
+        }
+        ctx->channels = c->ch_layout.nb_channels;
+    } else {
+        av_log(avctx, AV_LOG_ERROR, "Unsupported codec specified!"
+               " Only PCM_S16LE and AC-3 are supported.\n");
         return -1;
     }
+
     if (ctx->dlo->EnableAudioOutput(bmdAudioSampleRate48kHz,
                                     bmdAudioSampleType16bitInteger,
-                                    c->ch_layout.nb_channels,
+                                    ctx->channels,
                                     bmdAudioOutputStreamTimestamped) != S_OK) {
         av_log(avctx, AV_LOG_ERROR, "Could not enable audio output!\n");
         return -1;
@@ -266,14 +280,41 @@ static int decklink_setup_audio(AVFormatContext *avctx, AVStream *st)
     }
 
     /* The device expects the sample rate to be fixed. */
-    avpriv_set_pts_info(st, 64, 1, c->sample_rate);
-    ctx->channels = c->ch_layout.nb_channels;
+    avpriv_set_pts_info(st, 64, 1, 48000);
 
     ctx->audio = 1;
 
     return 0;
 }
 
+static int create_s337_payload(AVPacket *pkt, enum AVCodecID codec_id, uint8_t **outbuf, int *outsize)
+{
+    int payload_size = pkt->size + 8;
+    uint16_t bitcount = pkt->size * 8;
+    uint8_t *s337_payload;
+    PutByteContext pb;
+    int i;
+
+    if (codec_id != AV_CODEC_ID_AC3)
+        return AVERROR(EINVAL);
+
+    /* Encapsulate AC3 syncframe into SMPTE 337 packet */
+    s337_payload = (uint8_t *) av_mallocz(payload_size);
+    if (s337_payload == NULL)
+        return AVERROR(ENOMEM);
+    bytestream2_init_writer(&pb, s337_payload, payload_size);
+    bytestream2_put_le16u(&pb, 0xf872); /* Sync word 1 */
+    bytestream2_put_le16u(&pb, 0x4e1f); /* Sync word 1 */
+    bytestream2_put_le16u(&pb, 0x0001); /* Burst Info, including data type (1=ac3) */
+    bytestream2_put_le16u(&pb, bitcount); /* Length code */
+    for (i = 0; i < pkt->size; i += 2)
+        bytestream2_put_le16u(&pb, (pkt->data[i] << 8) | pkt->data[i+1]);
+
+    *outsize = payload_size;
+    *outbuf = s337_payload;
+    return 0;
+}
+
 av_cold int ff_decklink_write_trailer(AVFormatContext *avctx)
 {
     struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data;
@@ -531,21 +572,40 @@ static int decklink_write_audio_packet(AVFormatContext *avctx, AVPacket *pkt)
 {
     struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data;
     struct decklink_ctx *ctx = (struct decklink_ctx *)cctx->ctx;
-    int sample_count = pkt->size / (ctx->channels << 1);
+    AVStream *st = avctx->streams[pkt->stream_index];
+    int sample_count;
     uint32_t buffered;
+    uint8_t *outbuf = NULL;
+    int ret = 0;
 
     ctx->dlo->GetBufferedAudioSampleFrameCount(&buffered);
     if (pkt->pts > 1 && !buffered)
         av_log(avctx, AV_LOG_WARNING, "There's no buffered audio."
                " Audio will misbehave!\n");
 
-    if (ctx->dlo->ScheduleAudioSamples(pkt->data, sample_count, pkt->pts,
+    if (st->codecpar->codec_id == AV_CODEC_ID_AC3) {
+        /* Encapsulate AC3 syncframe into SMPTE 337 packet */
+        int outbuf_size;
+        ret = create_s337_payload(pkt, st->codecpar->codec_id,
+                                  &outbuf, &outbuf_size);
+        if (ret)
+            return ret;
+        sample_count = outbuf_size / 4;
+    } else {
+        sample_count = pkt->size / (ctx->channels << 1);
+        outbuf = pkt->data;
+    }
+
+    if (ctx->dlo->ScheduleAudioSamples(outbuf, sample_count, pkt->pts,
                                        bmdAudioSampleRate48kHz, NULL) != S_OK) {
         av_log(avctx, AV_LOG_ERROR, "Could not schedule audio samples.\n");
-        return AVERROR(EIO);
+        ret = AVERROR(EIO);
     }
 
-    return 0;
+    if (st->codecpar->codec_id == AV_CODEC_ID_AC3)
+        av_freep(&outbuf);
+
+    return ret;
 }
 
 extern "C" {
-- 
2.35.1.655.ga68dfadae5

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2 2/2] decklink: Add support for compressed AC-3 output over SDI

[Marton Balint]

On Fri, 17 Mar 2023, Devin Heitmueller wrote:

> Extend the decklink output to include support for compressed AC-3,
> encapsulated using the SMPTE ST 377:2015 standard.
>
> This functionality can be exercised by using the "copy" codec when
> the input audio stream is AC-3.  For example:
>
> ./ffmpeg -i ~/foo.ts -codec:a copy -f decklink 'UltraStudio Mini Monitor'
>
> Note that the default behavior continues to be to do PCM output,
> which means without specifying the copy codec a stream containing
> AC-3 will be decoded and downmixed to stereo audio before output.
>
> Thanks to Marton Balint for providing feedback.
>
> Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
> ---
> libavdevice/decklink_enc.cpp | 90 ++++++++++++++++++++++++++++++------
> 1 file changed, 75 insertions(+), 15 deletions(-)
>
> diff --git a/libavdevice/decklink_enc.cpp b/libavdevice/decklink_enc.cpp
> index 8d423f6b6e..8d80f00247 100644
> --- a/libavdevice/decklink_enc.cpp
> +++ b/libavdevice/decklink_enc.cpp
> @@ -32,6 +32,7 @@ extern "C" {
>
> extern "C" {
> #include "libavformat/avformat.h"
> +#include "libavcodec/bytestream.h"
> #include "libavutil/internal.h"
> #include "libavutil/imgutils.h"
> #include "avdevice.h"
> @@ -243,19 +244,32 @@ static int decklink_setup_audio(AVFormatContext *avctx, AVStream *st)
>         av_log(avctx, AV_LOG_ERROR, "Only one audio stream is supported!\n");
>         return -1;
>     }
> -    if (c->sample_rate != 48000) {
> -        av_log(avctx, AV_LOG_ERROR, "Unsupported sample rate!"
> -               " Only 48kHz is supported.\n");
> -        return -1;
> -    }
> -    if (c->ch_layout.nb_channels != 2 && c->ch_layout.nb_channels != 8 && c->ch_layout.nb_channels != 16) {
> -        av_log(avctx, AV_LOG_ERROR, "Unsupported number of channels!"
> -               " Only 2, 8 or 16 channels are supported.\n");
> +
> +    if (c->codec_id == AV_CODEC_ID_AC3) {
> +        /* Regardless of the number of channels in the codec, we're only
> +           using 2 SDI audio channels at 48000Hz */
> +        ctx->channels = 2;
> +    } else if (c->codec_id == AV_CODEC_ID_PCM_S16LE) {
> +        if (c->sample_rate != 48000) {
> +            av_log(avctx, AV_LOG_ERROR, "Unsupported sample rate!"
> +                   " Only 48kHz is supported.\n");
> +            return -1;
> +        }
> +        if (c->ch_layout.nb_channels != 2 && c->ch_layout.nb_channels != 8 && c->ch_layout.nb_channels != 16) {
> +            av_log(avctx, AV_LOG_ERROR, "Unsupported number of channels!"
> +                   " Only 2, 8 or 16 channels are supported.\n");
> +            return -1;
> +        }
> +        ctx->channels = c->ch_layout.nb_channels;
> +    } else {
> +        av_log(avctx, AV_LOG_ERROR, "Unsupported codec specified!"
> +               " Only PCM_S16LE and AC-3 are supported.\n");
>         return -1;
>     }
> +
>     if (ctx->dlo->EnableAudioOutput(bmdAudioSampleRate48kHz,
>                                     bmdAudioSampleType16bitInteger,
> -                                    c->ch_layout.nb_channels,
> +                                    ctx->channels,
>                                     bmdAudioOutputStreamTimestamped) != S_OK) {
>         av_log(avctx, AV_LOG_ERROR, "Could not enable audio output!\n");
>         return -1;
> @@ -266,14 +280,41 @@ static int decklink_setup_audio(AVFormatContext *avctx, AVStream *st)
>     }
>
>     /* The device expects the sample rate to be fixed. */
> -    avpriv_set_pts_info(st, 64, 1, c->sample_rate);
> -    ctx->channels = c->ch_layout.nb_channels;
> +    avpriv_set_pts_info(st, 64, 1, 48000);
>
>     ctx->audio = 1;
>
>     return 0;
> }
>
> +static int create_s337_payload(AVPacket *pkt, enum AVCodecID codec_id, uint8_t **outbuf, int *outsize)
> +{
> +    int payload_size = pkt->size + 8;
> +    uint16_t bitcount = pkt->size * 8;
> +    uint8_t *s337_payload;
> +    PutByteContext pb;
> +    int i;
> +
> +    if (codec_id != AV_CODEC_ID_AC3)
> +        return AVERROR(EINVAL);

Maybe some sanity check here for pkt->size upper limit to avoid overflows?

> +
> +    /* Encapsulate AC3 syncframe into SMPTE 337 packet */
> +    s337_payload = (uint8_t *) av_mallocz(payload_size);

Why not simply av_malloc?

> +    if (s337_payload == NULL)
> +        return AVERROR(ENOMEM);
> +    bytestream2_init_writer(&pb, s337_payload, payload_size);
> +    bytestream2_put_le16u(&pb, 0xf872); /* Sync word 1 */
> +    bytestream2_put_le16u(&pb, 0x4e1f); /* Sync word 1 */
> +    bytestream2_put_le16u(&pb, 0x0001); /* Burst Info, including data type (1=ac3) */
> +    bytestream2_put_le16u(&pb, bitcount); /* Length code */
> +    for (i = 0; i < pkt->size; i += 2)

for (int i =

> +        bytestream2_put_le16u(&pb, (pkt->data[i] << 8) | pkt->data[i+1]);
> +
> +    *outsize = payload_size;
> +    *outbuf = s337_payload;
> +    return 0;
> +}
> +
> av_cold int ff_decklink_write_trailer(AVFormatContext *avctx)
> {
>     struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data;
> @@ -531,21 +572,40 @@ static int decklink_write_audio_packet(AVFormatContext *avctx, AVPacket *pkt)
> {
>     struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data;
>     struct decklink_ctx *ctx = (struct decklink_ctx *)cctx->ctx;
> -    int sample_count = pkt->size / (ctx->channels << 1);
> +    AVStream *st = avctx->streams[pkt->stream_index];
> +    int sample_count;
>     uint32_t buffered;
> +    uint8_t *outbuf = NULL;
> +    int ret = 0;
>
>     ctx->dlo->GetBufferedAudioSampleFrameCount(&buffered);
>     if (pkt->pts > 1 && !buffered)
>         av_log(avctx, AV_LOG_WARNING, "There's no buffered audio."
>                " Audio will misbehave!\n");
>
> -    if (ctx->dlo->ScheduleAudioSamples(pkt->data, sample_count, pkt->pts,
> +    if (st->codecpar->codec_id == AV_CODEC_ID_AC3) {
> +        /* Encapsulate AC3 syncframe into SMPTE 337 packet */
> +        int outbuf_size;
> +        ret = create_s337_payload(pkt, st->codecpar->codec_id,
> +                                  &outbuf, &outbuf_size);
> +        if (ret)

if (ret < 0) is preferred

> +            return ret;
> +        sample_count = outbuf_size / 4;
> +    } else {
> +        sample_count = pkt->size / (ctx->channels << 1);
> +        outbuf = pkt->data;
> +    }
> +
> +    if (ctx->dlo->ScheduleAudioSamples(outbuf, sample_count, pkt->pts,
>                                        bmdAudioSampleRate48kHz, NULL) != S_OK) {
>         av_log(avctx, AV_LOG_ERROR, "Could not schedule audio samples.\n");
> -        return AVERROR(EIO);
> +        ret = AVERROR(EIO);
>     }
>
> -    return 0;
> +    if (st->codecpar->codec_id == AV_CODEC_ID_AC3)
> +        av_freep(&outbuf);
> +
> +    return ret;
> }
>

Thanks,
Marton
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2 1/2] avcodec: Fix warnings with signed/unsigned compare in bitstream.h

[Andreas Rheinhardt]

Devin Heitmueller:
> When including the header in decklink_enc.cpp it would be fed
> through the C++ compiler rather than the C compiler, which has
> more strict warnings when comparing signed/unsigned values.
> 
> Make the local variables unsigned to match the arguments they are
> being passed for those functions.
> 
> Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
> ---
>  libavcodec/bytestream.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/bytestream.h b/libavcodec/bytestream.h
> index d0033f14f3..67080604b9 100644
> --- a/libavcodec/bytestream.h
> +++ b/libavcodec/bytestream.h
> @@ -180,7 +180,7 @@ static av_always_inline void bytestream2_skipu(GetByteContext *g,
>  static av_always_inline void bytestream2_skip_p(PutByteContext *p,
>                                                  unsigned int size)
>  {
> -    int size2;
> +    unsigned int size2;
>      if (p->eof)
>          return;
>      size2 = FFMIN(p->buffer_end - p->buffer, size);
> @@ -268,7 +268,7 @@ static av_always_inline unsigned int bytestream2_get_buffer(GetByteContext *g,
>                                                              uint8_t *dst,
>                                                              unsigned int size)
>  {
> -    int size2 = FFMIN(g->buffer_end - g->buffer, size);
> +    unsigned int size2 = FFMIN(g->buffer_end - g->buffer, size);
>      memcpy(dst, g->buffer, size2);
>      g->buffer += size2;
>      return size2;
> @@ -287,7 +287,7 @@ static av_always_inline unsigned int bytestream2_put_buffer(PutByteContext *p,
>                                                              const uint8_t *src,
>                                                              unsigned int size)
>  {
> -    int size2;
> +    unsigned int size2;
>      if (p->eof)
>          return 0;
>      size2 = FFMIN(p->buffer_end - p->buffer, size);
> @@ -311,7 +311,7 @@ static av_always_inline void bytestream2_set_buffer(PutByteContext *p,
>                                                      const uint8_t c,
>                                                      unsigned int size)
>  {
> -    int size2;
> +    unsigned int size2;
>      if (p->eof)
>          return;
>      size2 = FFMIN(p->buffer_end - p->buffer, size);
> @@ -348,7 +348,7 @@ static av_always_inline unsigned int bytestream2_copy_buffer(PutByteContext *p,
>                                                               GetByteContext *g,
>                                                               unsigned int size)
>  {
> -    int size2;
> +    unsigned int size2;
>  
>      if (p->eof)
>          return 0;

The bytestream APIs are allowed to overread if the buffer is padded and
the user manages this himself. So you are not allowed to presume that
g->buffer_end - g->buffer is positive.

- Andreas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2 1/2] avcodec: Fix warnings with signed/unsigned compare in bitstream.h

[Marton Balint]

On Sat, 25 Mar 2023, Andreas Rheinhardt wrote:

> Devin Heitmueller:
>> When including the header in decklink_enc.cpp it would be fed
>> through the C++ compiler rather than the C compiler, which has
>> more strict warnings when comparing signed/unsigned values.
>>
>> Make the local variables unsigned to match the arguments they are
>> being passed for those functions.
>>
>> Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
>> ---
>>  libavcodec/bytestream.h | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavcodec/bytestream.h b/libavcodec/bytestream.h
>> index d0033f14f3..67080604b9 100644
>> --- a/libavcodec/bytestream.h
>> +++ b/libavcodec/bytestream.h
>> @@ -180,7 +180,7 @@ static av_always_inline void bytestream2_skipu(GetByteContext *g,
>>  static av_always_inline void bytestream2_skip_p(PutByteContext *p,
>>                                                  unsigned int size)
>>  {
>> -    int size2;
>> +    unsigned int size2;
>>      if (p->eof)
>>          return;
>>      size2 = FFMIN(p->buffer_end - p->buffer, size);
>> @@ -268,7 +268,7 @@ static av_always_inline unsigned int bytestream2_get_buffer(GetByteContext *g,
>>                                                              uint8_t *dst,
>>                                                              unsigned int size)
>>  {
>> -    int size2 = FFMIN(g->buffer_end - g->buffer, size);
>> +    unsigned int size2 = FFMIN(g->buffer_end - g->buffer, size);
>>      memcpy(dst, g->buffer, size2);
>>      g->buffer += size2;
>>      return size2;
>> @@ -287,7 +287,7 @@ static av_always_inline unsigned int bytestream2_put_buffer(PutByteContext *p,
>>                                                              const uint8_t *src,
>>                                                              unsigned int size)
>>  {
>> -    int size2;
>> +    unsigned int size2;
>>      if (p->eof)
>>          return 0;
>>      size2 = FFMIN(p->buffer_end - p->buffer, size);
>> @@ -311,7 +311,7 @@ static av_always_inline void bytestream2_set_buffer(PutByteContext *p,
>>                                                      const uint8_t c,
>>                                                      unsigned int size)
>>  {
>> -    int size2;
>> +    unsigned int size2;
>>      if (p->eof)
>>          return;
>>      size2 = FFMIN(p->buffer_end - p->buffer, size);
>> @@ -348,7 +348,7 @@ static av_always_inline unsigned int bytestream2_copy_buffer(PutByteContext *p,
>>                                                               GetByteContext *g,
>>                                                               unsigned int size)
>>  {
>> -    int size2;
>> +    unsigned int size2;
>>
>>      if (p->eof)
>>          return 0;
>
> The bytestream APIs are allowed to overread if the buffer is padded and
> the user manages this himself. So you are not allowed to presume that
> g->buffer_end - g->buffer is positive.

I am not sure if overread/overwrote is a supported state for these 
functions. As far as I see bytestream2_get_buffer, bytestream2_put_buffer, 
bytestream2_copy_buffer and bytestream2_set_buffer just crashes if 
buffer_end < buffer because sooner or later memcpy/memset gets a negative 
value. There are no special checks to handle it.

Regards,
Marton
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2 1/2] avcodec: Fix warnings with signed/unsigned compare in bitstream.h

[Devin Heitmueller]

On Sat, Mar 25, 2023 at 1:10 PM Marton Balint <cus@passwd.hu> wrote:
> I am not sure if overread/overwrote is a supported state for these
> functions. As far as I see bytestream2_get_buffer, bytestream2_put_buffer,
> bytestream2_copy_buffer and bytestream2_set_buffer just crashes if
> buffer_end < buffer because sooner or later memcpy/memset gets a negative
> value. There are no special checks to handle it.

This was the conclusion I came to as well.  I couldn't imagine a case
where it would ever actually work, since prior to my patch in every
case it results in a call to memcpy() with a negative length argument.

Devin

-- 
Devin Heitmueller, Senior Software Engineer
LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com  e: devin.heitmueller@ltnglobal.com
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2 1/2] avcodec: Fix warnings with signed/unsigned compare in bitstream.h

[Andreas Rheinhardt]

Marton Balint:
> 
> 
> On Sat, 25 Mar 2023, Andreas Rheinhardt wrote:
> 
>> Devin Heitmueller:
>>> When including the header in decklink_enc.cpp it would be fed
>>> through the C++ compiler rather than the C compiler, which has
>>> more strict warnings when comparing signed/unsigned values.
>>>
>>> Make the local variables unsigned to match the arguments they are
>>> being passed for those functions.
>>>
>>> Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
>>> ---
>>>  libavcodec/bytestream.h | 10 +++++-----
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/libavcodec/bytestream.h b/libavcodec/bytestream.h
>>> index d0033f14f3..67080604b9 100644
>>> --- a/libavcodec/bytestream.h
>>> +++ b/libavcodec/bytestream.h
>>> @@ -180,7 +180,7 @@ static av_always_inline void
>>> bytestream2_skipu(GetByteContext *g,
>>>  static av_always_inline void bytestream2_skip_p(PutByteContext *p,
>>>                                                  unsigned int size)
>>>  {
>>> -    int size2;
>>> +    unsigned int size2;
>>>      if (p->eof)
>>>          return;
>>>      size2 = FFMIN(p->buffer_end - p->buffer, size);
>>> @@ -268,7 +268,7 @@ static av_always_inline unsigned int
>>> bytestream2_get_buffer(GetByteContext *g,
>>>                                                              uint8_t
>>> *dst,
>>>                                                              unsigned
>>> int size)
>>>  {
>>> -    int size2 = FFMIN(g->buffer_end - g->buffer, size);
>>> +    unsigned int size2 = FFMIN(g->buffer_end - g->buffer, size);
>>>      memcpy(dst, g->buffer, size2);
>>>      g->buffer += size2;
>>>      return size2;
>>> @@ -287,7 +287,7 @@ static av_always_inline unsigned int
>>> bytestream2_put_buffer(PutByteContext *p,
>>>                                                              const
>>> uint8_t *src,
>>>                                                              unsigned
>>> int size)
>>>  {
>>> -    int size2;
>>> +    unsigned int size2;
>>>      if (p->eof)
>>>          return 0;
>>>      size2 = FFMIN(p->buffer_end - p->buffer, size);
>>> @@ -311,7 +311,7 @@ static av_always_inline void
>>> bytestream2_set_buffer(PutByteContext *p,
>>>                                                      const uint8_t c,
>>>                                                      unsigned int size)
>>>  {
>>> -    int size2;
>>> +    unsigned int size2;
>>>      if (p->eof)
>>>          return;
>>>      size2 = FFMIN(p->buffer_end - p->buffer, size);
>>> @@ -348,7 +348,7 @@ static av_always_inline unsigned int
>>> bytestream2_copy_buffer(PutByteContext *p,
>>>                                                              
>>> GetByteContext *g,
>>>                                                              
>>> unsigned int size)
>>>  {
>>> -    int size2;
>>> +    unsigned int size2;
>>>
>>>      if (p->eof)
>>>          return 0;
>>
>> The bytestream APIs are allowed to overread if the buffer is padded and
>> the user manages this himself. So you are not allowed to presume that
>> g->buffer_end - g->buffer is positive.
> 
> I am not sure if overread/overwrote is a supported state for these
> functions. As far as I see bytestream2_get_buffer,
> bytestream2_put_buffer, bytestream2_copy_buffer and
> bytestream2_set_buffer just crashes if buffer_end < buffer because
> sooner or later memcpy/memset gets a negative value. There are no
> special checks to handle it.
> 

True. Seems like this was never a supported case. Objection lifted.

- Andreas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2 2/2] decklink: Add support for compressed AC-3 output over SDI

[Devin Heitmueller]

On Fri, Mar 24, 2023 at 5:07 PM Marton Balint <cus@passwd.hu> wrote:
>
>
>
> On Fri, 17 Mar 2023, Devin Heitmueller wrote:
>
> > Extend the decklink output to include support for compressed AC-3,
> > encapsulated using the SMPTE ST 377:2015 standard.
> >
> > This functionality can be exercised by using the "copy" codec when
> > the input audio stream is AC-3.  For example:
> >
> > ./ffmpeg -i ~/foo.ts -codec:a copy -f decklink 'UltraStudio Mini Monitor'
> >
> > Note that the default behavior continues to be to do PCM output,
> > which means without specifying the copy codec a stream containing
> > AC-3 will be decoded and downmixed to stereo audio before output.
> >
> > Thanks to Marton Balint for providing feedback.
> >
> > Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
> > ---
> > libavdevice/decklink_enc.cpp | 90 ++++++++++++++++++++++++++++++------
> > 1 file changed, 75 insertions(+), 15 deletions(-)
> >
> > diff --git a/libavdevice/decklink_enc.cpp b/libavdevice/decklink_enc.cpp
> > index 8d423f6b6e..8d80f00247 100644
> > --- a/libavdevice/decklink_enc.cpp
> > +++ b/libavdevice/decklink_enc.cpp
> > @@ -32,6 +32,7 @@ extern "C" {
> >
> > extern "C" {
> > #include "libavformat/avformat.h"
> > +#include "libavcodec/bytestream.h"
> > #include "libavutil/internal.h"
> > #include "libavutil/imgutils.h"
> > #include "avdevice.h"
> > @@ -243,19 +244,32 @@ static int decklink_setup_audio(AVFormatContext *avctx, AVStream *st)
> >         av_log(avctx, AV_LOG_ERROR, "Only one audio stream is supported!\n");
> >         return -1;
> >     }
> > -    if (c->sample_rate != 48000) {
> > -        av_log(avctx, AV_LOG_ERROR, "Unsupported sample rate!"
> > -               " Only 48kHz is supported.\n");
> > -        return -1;
> > -    }
> > -    if (c->ch_layout.nb_channels != 2 && c->ch_layout.nb_channels != 8 && c->ch_layout.nb_channels != 16) {
> > -        av_log(avctx, AV_LOG_ERROR, "Unsupported number of channels!"
> > -               " Only 2, 8 or 16 channels are supported.\n");
> > +
> > +    if (c->codec_id == AV_CODEC_ID_AC3) {
> > +        /* Regardless of the number of channels in the codec, we're only
> > +           using 2 SDI audio channels at 48000Hz */
> > +        ctx->channels = 2;
> > +    } else if (c->codec_id == AV_CODEC_ID_PCM_S16LE) {
> > +        if (c->sample_rate != 48000) {
> > +            av_log(avctx, AV_LOG_ERROR, "Unsupported sample rate!"
> > +                   " Only 48kHz is supported.\n");
> > +            return -1;
> > +        }
> > +        if (c->ch_layout.nb_channels != 2 && c->ch_layout.nb_channels != 8 && c->ch_layout.nb_channels != 16) {
> > +            av_log(avctx, AV_LOG_ERROR, "Unsupported number of channels!"
> > +                   " Only 2, 8 or 16 channels are supported.\n");
> > +            return -1;
> > +        }
> > +        ctx->channels = c->ch_layout.nb_channels;
> > +    } else {
> > +        av_log(avctx, AV_LOG_ERROR, "Unsupported codec specified!"
> > +               " Only PCM_S16LE and AC-3 are supported.\n");
> >         return -1;
> >     }
> > +
> >     if (ctx->dlo->EnableAudioOutput(bmdAudioSampleRate48kHz,
> >                                     bmdAudioSampleType16bitInteger,
> > -                                    c->ch_layout.nb_channels,
> > +                                    ctx->channels,
> >                                     bmdAudioOutputStreamTimestamped) != S_OK) {
> >         av_log(avctx, AV_LOG_ERROR, "Could not enable audio output!\n");
> >         return -1;
> > @@ -266,14 +280,41 @@ static int decklink_setup_audio(AVFormatContext *avctx, AVStream *st)
> >     }
> >
> >     /* The device expects the sample rate to be fixed. */
> > -    avpriv_set_pts_info(st, 64, 1, c->sample_rate);
> > -    ctx->channels = c->ch_layout.nb_channels;
> > +    avpriv_set_pts_info(st, 64, 1, 48000);
> >
> >     ctx->audio = 1;
> >
> >     return 0;
> > }
> >
> > +static int create_s337_payload(AVPacket *pkt, enum AVCodecID codec_id, uint8_t **outbuf, int *outsize)
> > +{
> > +    int payload_size = pkt->size + 8;
> > +    uint16_t bitcount = pkt->size * 8;
> > +    uint8_t *s337_payload;
> > +    PutByteContext pb;
> > +    int i;
> > +
> > +    if (codec_id != AV_CODEC_ID_AC3)
> > +        return AVERROR(EINVAL);
>
> Maybe some sanity check here for pkt->size upper limit to avoid overflows?
>
> > +
> > +    /* Encapsulate AC3 syncframe into SMPTE 337 packet */
> > +    s337_payload = (uint8_t *) av_mallocz(payload_size);
>
> Why not simply av_malloc?
>
> > +    if (s337_payload == NULL)
> > +        return AVERROR(ENOMEM);
> > +    bytestream2_init_writer(&pb, s337_payload, payload_size);
> > +    bytestream2_put_le16u(&pb, 0xf872); /* Sync word 1 */
> > +    bytestream2_put_le16u(&pb, 0x4e1f); /* Sync word 1 */
> > +    bytestream2_put_le16u(&pb, 0x0001); /* Burst Info, including data type (1=ac3) */
> > +    bytestream2_put_le16u(&pb, bitcount); /* Length code */
> > +    for (i = 0; i < pkt->size; i += 2)
>
> for (int i =
>
> > +        bytestream2_put_le16u(&pb, (pkt->data[i] << 8) | pkt->data[i+1]);
> > +
> > +    *outsize = payload_size;
> > +    *outbuf = s337_payload;
> > +    return 0;
> > +}
> > +
> > av_cold int ff_decklink_write_trailer(AVFormatContext *avctx)
> > {
> >     struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data;
> > @@ -531,21 +572,40 @@ static int decklink_write_audio_packet(AVFormatContext *avctx, AVPacket *pkt)
> > {
> >     struct decklink_cctx *cctx = (struct decklink_cctx *)avctx->priv_data;
> >     struct decklink_ctx *ctx = (struct decklink_ctx *)cctx->ctx;
> > -    int sample_count = pkt->size / (ctx->channels << 1);
> > +    AVStream *st = avctx->streams[pkt->stream_index];
> > +    int sample_count;
> >     uint32_t buffered;
> > +    uint8_t *outbuf = NULL;
> > +    int ret = 0;
> >
> >     ctx->dlo->GetBufferedAudioSampleFrameCount(&buffered);
> >     if (pkt->pts > 1 && !buffered)
> >         av_log(avctx, AV_LOG_WARNING, "There's no buffered audio."
> >                " Audio will misbehave!\n");
> >
> > -    if (ctx->dlo->ScheduleAudioSamples(pkt->data, sample_count, pkt->pts,
> > +    if (st->codecpar->codec_id == AV_CODEC_ID_AC3) {
> > +        /* Encapsulate AC3 syncframe into SMPTE 337 packet */
> > +        int outbuf_size;
> > +        ret = create_s337_payload(pkt, st->codecpar->codec_id,
> > +                                  &outbuf, &outbuf_size);
> > +        if (ret)
>
> if (ret < 0) is preferred
>
> > +            return ret;
> > +        sample_count = outbuf_size / 4;
> > +    } else {
> > +        sample_count = pkt->size / (ctx->channels << 1);
> > +        outbuf = pkt->data;
> > +    }
> > +
> > +    if (ctx->dlo->ScheduleAudioSamples(outbuf, sample_count, pkt->pts,
> >                                        bmdAudioSampleRate48kHz, NULL) != S_OK) {
> >         av_log(avctx, AV_LOG_ERROR, "Could not schedule audio samples.\n");
> > -        return AVERROR(EIO);
> > +        ret = AVERROR(EIO);
> >     }
> >
> > -    return 0;
> > +    if (st->codecpar->codec_id == AV_CODEC_ID_AC3)
> > +        av_freep(&outbuf);
> > +
> > +    return ret;
> > }
> >
>
> Thanks,
> Marton

Thanks for your feedback.  A revised patch reflecting your changes
will be sent to the mailing list shortly.

Devin

-- 
Devin Heitmueller, Senior Software Engineer
LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com  e: devin.heitmueller@ltnglobal.com
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2 1/2] avcodec: Fix warnings with signed/unsigned compare in bitstream.h

[Marton Balint]

On Mon, 27 Mar 2023, Andreas Rheinhardt wrote:

> Marton Balint:
>> 
>> 
>> On Sat, 25 Mar 2023, Andreas Rheinhardt wrote:
>> 
>>> Devin Heitmueller:
>>>> When including the header in decklink_enc.cpp it would be fed
>>>> through the C++ compiler rather than the C compiler, which has
>>>> more strict warnings when comparing signed/unsigned values.
>>>>
>>>> Make the local variables unsigned to match the arguments they are
>>>> being passed for those functions.
>>>>
>>>> Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
>>>> ---
>>>>  libavcodec/bytestream.h | 10 +++++-----
>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/libavcodec/bytestream.h b/libavcodec/bytestream.h
>>>> index d0033f14f3..67080604b9 100644
>>>> --- a/libavcodec/bytestream.h
>>>> +++ b/libavcodec/bytestream.h
>>>> @@ -180,7 +180,7 @@ static av_always_inline void
>>>> bytestream2_skipu(GetByteContext *g,
>>>>  static av_always_inline void bytestream2_skip_p(PutByteContext *p,
>>>>                                                  unsigned int size)
>>>>  {
>>>> -    int size2;
>>>> +    unsigned int size2;
>>>>      if (p->eof)
>>>>          return;
>>>>      size2 = FFMIN(p->buffer_end - p->buffer, size);
>>>> @@ -268,7 +268,7 @@ static av_always_inline unsigned int
>>>> bytestream2_get_buffer(GetByteContext *g,
>>>>                                                              uint8_t
>>>> *dst,
>>>>                                                              unsigned
>>>> int size)
>>>>  {
>>>> -    int size2 = FFMIN(g->buffer_end - g->buffer, size);
>>>> +    unsigned int size2 = FFMIN(g->buffer_end - g->buffer, size);
>>>>      memcpy(dst, g->buffer, size2);
>>>>      g->buffer += size2;
>>>>      return size2;
>>>> @@ -287,7 +287,7 @@ static av_always_inline unsigned int
>>>> bytestream2_put_buffer(PutByteContext *p,
>>>>                                                              const
>>>> uint8_t *src,
>>>>                                                              unsigned
>>>> int size)
>>>>  {
>>>> -    int size2;
>>>> +    unsigned int size2;
>>>>      if (p->eof)
>>>>          return 0;
>>>>      size2 = FFMIN(p->buffer_end - p->buffer, size);
>>>> @@ -311,7 +311,7 @@ static av_always_inline void
>>>> bytestream2_set_buffer(PutByteContext *p,
>>>>                                                      const uint8_t c,
>>>>                                                      unsigned int size)
>>>>  {
>>>> -    int size2;
>>>> +    unsigned int size2;
>>>>      if (p->eof)
>>>>          return;
>>>>      size2 = FFMIN(p->buffer_end - p->buffer, size);
>>>> @@ -348,7 +348,7 @@ static av_always_inline unsigned int
>>>> bytestream2_copy_buffer(PutByteContext *p,
>>>>                                                              
>>>> GetByteContext *g,
>>>>                                                              
>>>> unsigned int size)
>>>>  {
>>>> -    int size2;
>>>> +    unsigned int size2;
>>>>
>>>>      if (p->eof)
>>>>          return 0;
>>>
>>> The bytestream APIs are allowed to overread if the buffer is padded and
>>> the user manages this himself. So you are not allowed to presume that
>>> g->buffer_end - g->buffer is positive.
>> 
>> I am not sure if overread/overwrote is a supported state for these
>> functions. As far as I see bytestream2_get_buffer,
>> bytestream2_put_buffer, bytestream2_copy_buffer and
>> bytestream2_set_buffer just crashes if buffer_end < buffer because
>> sooner or later memcpy/memset gets a negative value. There are no
>> special checks to handle it.
>> 
>
> True. Seems like this was never a supported case. Objection lifted.

Ok, will apply.

Regards,
Marton
_______________________________________________
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".