[FFmpeg-devel] [PATCH] lavc/libopenh264: Drop openh264 runtime version checks

2023-12-08 Thread Kalev Lember
Years ago, openh264 releases often changed their ABI without changing
the library soname. To avoid running into ABI issues, a version check
was added to lavc libopenh264 code to error out at runtime in case the
build time and runtime openh264 versions don't match.

This should no longer be an issue with newer openh264 releases and we
can drop the runtime version check and rely on upstream doing the right
thing and bump the library soname if the ABI changes, similar to how
other libraries are consumed in ffmpeg.

Almost all major distributions now include openh264 and this means there
are more eyes on ABI changes and issues are discovered and reported
quickly. See e.g. https://github.com/cisco/openh264/issues/3564 where an
ABI issue was quickly discovered and fixed.

Relaxing the check allows downstream distributions to build ffmpeg
against e.g. openh264 2.3.1 and ship an update to ABI-compatible
openh264 2.4.0, without needing to coordinate a lock step update between
ffmpeg and openh264 (which can be difficult if openh264 is distributed
by Cisco and ffmpeg comes from the distro, such as is the case for
Fedora).

Signed-off-by: Kalev Lember 
---
 libavcodec/libopenh264.c| 15 ---
 libavcodec/libopenh264.h|  2 --
 libavcodec/libopenh264dec.c |  4 
 libavcodec/libopenh264enc.c |  4 
 4 files changed, 25 deletions(-)

diff --git a/libavcodec/libopenh264.c b/libavcodec/libopenh264.c
index 0f6d28ed88..c80c85ea8b 100644
--- a/libavcodec/libopenh264.c
+++ b/libavcodec/libopenh264.c
@@ -46,18 +46,3 @@ void ff_libopenh264_trace_callback(void *ctx, int level, 
const char *msg)
 int equiv_ffmpeg_log_level = libopenh264_to_ffmpeg_log_level(level);
 av_log(ctx, equiv_ffmpeg_log_level, "%s\n", msg);
 }
-
-int ff_libopenh264_check_version(void *logctx)
-{
-// Mingw GCC < 4.7 on x86_32 uses an incorrect/buggy ABI for the 
WelsGetCodecVersion
-// function (for functions returning larger structs), thus skip the check 
in those
-// configurations.
-#if !defined(_WIN32) || !defined(__GNUC__) || !ARCH_X86_32 || 
AV_GCC_VERSION_AT_LEAST(4, 7)
-OpenH264Version libver = WelsGetCodecVersion();
-if (memcmp(&libver, &g_stCodecVersion, sizeof(libver))) {
-av_log(logctx, AV_LOG_ERROR, "Incorrect library version loaded\n");
-return AVERROR(EINVAL);
-}
-#endif
-return 0;
-}
diff --git a/libavcodec/libopenh264.h b/libavcodec/libopenh264.h
index dbb9c5d429..0b462d6fdc 100644
--- a/libavcodec/libopenh264.h
+++ b/libavcodec/libopenh264.h
@@ -34,6 +34,4 @@
 
 void ff_libopenh264_trace_callback(void *ctx, int level, const char *msg);
 
-int ff_libopenh264_check_version(void *logctx);
-
 #endif /* AVCODEC_LIBOPENH264_H */
diff --git a/libavcodec/libopenh264dec.c b/libavcodec/libopenh264dec.c
index 7d650ae03e..b6a9bba2dc 100644
--- a/libavcodec/libopenh264dec.c
+++ b/libavcodec/libopenh264dec.c
@@ -52,13 +52,9 @@ static av_cold int svc_decode_init(AVCodecContext *avctx)
 {
 SVCContext *s = avctx->priv_data;
 SDecodingParam param = { 0 };
-int err;
 int log_level;
 WelsTraceCallback callback_function;
 
-if ((err = ff_libopenh264_check_version(avctx)) < 0)
-return AVERROR_DECODER_NOT_FOUND;
-
 if (WelsCreateDecoder(&s->decoder)) {
 av_log(avctx, AV_LOG_ERROR, "Unable to create decoder\n");
 return AVERROR_UNKNOWN;
diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
index f518d0894e..6f231d22b2 100644
--- a/libavcodec/libopenh264enc.c
+++ b/libavcodec/libopenh264enc.c
@@ -110,14 +110,10 @@ static av_cold int svc_encode_init(AVCodecContext *avctx)
 {
 SVCContext *s = avctx->priv_data;
 SEncParamExt param = { 0 };
-int err;
 int log_level;
 WelsTraceCallback callback_function;
 AVCPBProperties *props;
 
-if ((err = ff_libopenh264_check_version(avctx)) < 0)
-return AVERROR_ENCODER_NOT_FOUND;
-
 if (WelsCreateSVCEncoder(&s->encoder)) {
 av_log(avctx, AV_LOG_ERROR, "Unable to create encoder\n");
 return AVERROR_UNKNOWN;
-- 
2.43.0

___
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] lavc/libopenh264: Drop openh264 runtime version checks

2023-12-08 Thread Martin Storsjö

Hi,

On Fri, 8 Dec 2023, Kalev Lember wrote:


This should no longer be an issue with newer openh264 releases and we
can drop the runtime version check and rely on upstream doing the right
thing and bump the library soname if the ABI changes, similar to how
other libraries are consumed in ffmpeg.

Almost all major distributions now include openh264 and this means there
are more eyes on ABI changes and issues are discovered and reported
quickly. See e.g. https://github.com/cisco/openh264/issues/3564 where an
ABI issue was quickly discovered and fixed.

Relaxing the check allows downstream distributions to build ffmpeg
against e.g. openh264 2.3.1 and ship an update to ABI-compatible
openh264 2.4.0, without needing to coordinate a lock step update between
ffmpeg and openh264 (which can be difficult if openh264 is distributed
by Cisco and ffmpeg comes from the distro, such as is the case for
Fedora).

Signed-off-by: Kalev Lember 
---
libavcodec/libopenh264.c| 15 ---
libavcodec/libopenh264.h|  2 --
libavcodec/libopenh264dec.c |  4 
libavcodec/libopenh264enc.c |  4 
4 files changed, 25 deletions(-)


I guess this seems reasonable to me, so LGTM.

The version check would be more relevant if we would be dlopening the 
OpenH264 library (which pretty much is what one has to do in order to take 
advantage of the patent offer from Cisco), but the libavcodec wrapper 
doesn't dlopen this library (and doing the dlopen dance for ffmpeg is kind 
of pointless, there's more of a point to it if individual apps want to 
integrate OpenH264 directly), so this should indeed be fine.


// Martin

___
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 10 bit support v5 1/3] avcodec/amfenc: Fixes the color information in the output.

2023-12-08 Thread Evgeny Pavlov
On Wed, Nov 29, 2023 at 11:57 AM Evgeny Pavlov  wrote:

> On Tue, Nov 28, 2023 at 8:13 PM Mark Thompson  wrote:
>
>> I upgraded to 23.11.1 and see no change - the colour information is still
>> missing in the header but not the stream, and the two different sequence
>> parameter sets are identical to what they were before the change.
>>
>> Can you share what your trace_headers output looks like for the
>> out-of-band and in-band parameter sets?  Are they identical for you?
>>
> Yes, it seems that they are identical for me and both have colour
> information (please find my output below).
> Is it possible to provide a video you tested? Probably I need to
> test the patch on your video input.
>
> Hi Mark, could you share which AMD hardware you use for testing? Your
hardware might use older driver version without the fix for missing color
information in the header
___
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] avutil/mem: always align by at least 32 bytes

2023-12-08 Thread Andreas Rheinhardt
Timo Rothenpieler:
> FFmpeg has instances of DECLARE_ALIGNED(32, ...) in a lot of structs,
> which then end up heap-allocated.
> By declaring any variable in a struct, or tree of structs, to be 32 byte
> aligned, it allows the compiler to safely assume the entire struct
> itself is also 32 byte aligned.
> 
> This might make the compiler emit code which straight up crashes or
> misbehaves in other ways, and at least in one instances is now
> documented to actually do (see ticket 10549 on trac).
> The issue there is that an unrelated variable in SingleChannelElement is
> declared to have an alignment of 32 bytes. So if the compiler does a copy
> in decode_cpe() with avx instructions, but ffmpeg is built with
> --disable-avx, this results in a crash, since the memory is only 16 byte
> aligned.
> Mind you, even if the compiler does not emit avx instructions, the code
> is still invalid and could misbehave. It just happens not to. Declaring
> any variable in a struct with a 32 byte alignment promises 32 byte
> alignment of the whole struct to the compiler.
> 
> Instead of now going through all instances of variables in structs
> being declared as 32 byte aligned, this patch bumps the minimum alignment
> to 32 bytes.
> ---
>  libavutil/mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavutil/mem.c b/libavutil/mem.c
> index 36b8940a0c..26a9b9753b 100644
> --- a/libavutil/mem.c
> +++ b/libavutil/mem.c
> @@ -62,7 +62,7 @@ void  free(void *ptr);
>  
>  #endif /* MALLOC_PREFIX */
>  
> -#define ALIGN (HAVE_AVX512 ? 64 : (HAVE_AVX ? 32 : 16))
> +#define ALIGN (HAVE_AVX512 ? 64 : 32)
>  
>  /* NOTE: if you want to override these functions with your own
>   * implementations (not recommended) you have to link libav* as

1. There is another way in which this can be triggered: Namely if one
uses a build with AVX, but combines it with a lavu built without it; it
is also triggerable on non-x86 (having an insufficiently aligned pointer
is always UB even if the CPU does not have instructions that would
benefit from the additional alignment). You should mention this in the
commit message.

2. This topic gave me headaches when creating RefStruct. I "solved" it
by (ab)using STRIDE_ALIGN which mimicks the alignment of av_malloc(),
thereby ensuring that RefStruct does not break lavc builds built with
the avx dsp functions enabled (but it does not guard against using a
lavu whose av_malloc() only provides less alignment).

3. There is a downside to your patch: It bumps alignment for non-x86
arches which wastes memory (and may make allocators slower). We could
fix this by modifying the 32-byte-alignment macros to only provide 16
byte alignment if the ARCH_ (and potentially the HAVE_) defines indicate
that no alignment bigger than 16 is needed.

- 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-requ...@ffmpeg.org with subject "unsubscribe".


Re: [FFmpeg-devel] [PATCH] lavc/libopenh264: Drop openh264 runtime version checks

2023-12-08 Thread Kalev Lember
On Fri, Dec 8, 2023 at 9:39 AM Martin Storsjö  wrote:

> I guess this seems reasonable to me, so LGTM.
>
> The version check would be more relevant if we would be dlopening the
> OpenH264 library (which pretty much is what one has to do in order to take
> advantage of the patent offer from Cisco), but the libavcodec wrapper
> doesn't dlopen this library (and doing the dlopen dance for ffmpeg is kind
> of pointless, there's more of a point to it if individual apps want to
> integrate OpenH264 directly), so this should indeed be fine.
>

Thanks!

As for dlopening, I think instead of version checks, it would make sense to
try to dlsym() all of the actual required symbols, and error out in init if
anything is missing. That should make it all super flexible and resilient
to e.g. struct size changes that would normally be an ABI change.

In Fedora, we are planning on changing things up a bit and starting to
build packages that link with openh264 against the "noopenh264" stub
implementation and replacing it at runtime with the actual openh264 library
downloaded directly from Cisco. Flathub flatpak runtimes already use that
approach and it seems to work well there. This should hopefully let us take
advantage of the Cisco patent grant and fit well in the build system
architecture that we have.

https://gitlab.com/freedesktop-sdk/noopenh264

-- 
Kalev
___
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 v2 0/3] Initial support for fragmented TTML muxing

2023-12-08 Thread Jan Ekström
On Fri, Sep 15, 2023 at 4:17 PM Jan Ekström  wrote:
>
> Changes compared to v1:
>
> * General rebase.
> * A FATE test was added, together with the extension of the "transcode"
>   test function to allow for dumping of packets' contents.
> * Simplified mov_write_ttml_document_from_queue's loop by getting
>   rid of `stop_at_current_packet`.
>
> This enables pushing TTML together with another track (usually video)
> as part of CMAF Ingest, as defined by the DASH-IF Live Media Ingest
> Protocol.
>
> Currently does not function well with just the subtitle track unless
> the API user explicitly requests fragmentation with a nullptr packet,
> as the generic fragmentation decision logic is based on tracks which
> do not require squashing.
>
> Currently does support overlapping subtitles, but the implementation
> utilizes another packet queue for it, which is probably not optimal.
> Recommendations on how to improve things are welcome.
>
> Jan
>

Ping.

As this is a piece of seemingly working functionality, I'd like to
understand whether people think subtitle-only fragmented MP4 documents
with more than one fragment is something that is required for this to
get merged.

Also I have changed an existing test function, so I'd like a note
whether people think this is an OK change.

Jan
___
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 v5 00/14] encoder AVCodecContext configuration side data

2023-12-08 Thread Jan Ekström
On Sun, Nov 26, 2023 at 9:58 PM Jan Ekström  wrote:
>
> Differences to v3:
> 1. rebased on top of current master
> 2. moved the addition of multiple side data entries from a generic
>av_frame_side_data_set_extend to avcodec as per request from James.
> 4. adopted various things noted by reviews
>
> Comparison URL (mostly configure and wrappers, avutil/frame.c):
> https://github.com/jeeb/ffmpeg/compare/avcodec_cll_mdcv_side_data_v4..avcodec_cll_mdcv_side_data_v5
>
> This patch set I've now been working for a while since I felt like it was 
> weird
> we couldn't pass through information such as static HDR metadata to encoders
> from decoded input. This initial version adds the necessary framework, as well
> as adds static HDR metadata support for libsvtav1, libx264 as well as libx265
> wrappers.
>
> An alternative to this would be to make encoders only properly initialize when
> they receive the first AVFrame, but that seems to be a bigger, nastier change
> than introducing an AVFrameSideDataSet in avctx as everything seems to
> presume that extradata etc are available after opening the encoder.
>
> Note: Any opinions on whether FFCodec or AVCodec should have
>   handled_side_data list, so that if format specific generic logic is
>   added, it could be checked whether the codec itself handles this side
>   data? This could also be utilized to list handled side data from f.ex.
>   `ffmpeg -h encoder=libsvtav1`.
>
> Jan

Ping.

I'd like to understand whether:

* people are fine with the struct which lets you pass things as a
single argument, or they would like to move all the helper functions
to counter and pointer.
* should the avcodec helper take in the struct/{counter,pointer}, or
should it instead take in a const AVFrame pointer?

as I'd like to start pulling this in, and move towards working on
implemented side data listing in AVCodecs, as well as adding generic
implementations for specific codecs (such as H.264/HEVC/AV1 having CBS
create side data generically, without the need of specific AVCodecs
implementing things),

Jan
___
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] lavc/libopenh264: Drop openh264 runtime version checks

2023-12-08 Thread Martin Storsjö

On Fri, 8 Dec 2023, Kalev Lember wrote:


As for dlopening, I think instead of version checks, it would make sense to
try to dlsym() all of the actual required symbols, and error out in init if
anything is missing. That should make it all super flexible and resilient to
e.g. struct size changes that would normally be an ABI change.


How would that help, if e.g. the SEncParamExt struct in svc_encode_init 
would change layout/size - which part would notice that change?



In Fedora, we are planning on changing things up a bit and starting to build
packages that link with openh264 against the "noopenh264" stub
implementation and replacing it at runtime with the actual openh264 library
downloaded directly from Cisco. Flathub flatpak runtimes already use that
approach and it seems to work well there. This should hopefully let us take
advantage of the Cisco patent grant and fit well in the build system
architecture that we have.


Ah, interesting, that sounds like a reasonable way to take advantage of 
that patent grant without having everybody to do the dlopening.


// Martin

___
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] lavc/libopenh264: Drop openh264 runtime version checks

2023-12-08 Thread Kalev Lember
On Fri, Dec 8, 2023 at 1:00 PM Martin Storsjö  wrote:

> On Fri, 8 Dec 2023, Kalev Lember wrote:
>
> > As for dlopening, I think instead of version checks, it would make sense
> to
> > try to dlsym() all of the actual required symbols, and error out in init
> if
> > anything is missing. That should make it all super flexible and
> resilient to
> > e.g. struct size changes that would normally be an ABI change.
>
> How would that help, if e.g. the SEncParamExt struct in svc_encode_init
> would change layout/size - which part would notice that change?
>

Ah, hm, I didn't think this through apparently :) This would indeed still
be an issue.

I guess maybe dlopening the soname version that matches the headers (e.g.
libopenh264.so.7) would work then? With the expectation that upstream bumps
soname whenever the struct layout/size changes.

-- 
Kalev
___
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 v2 3/3] avformat/movenc: add support for fragmented TTML muxing

2023-12-08 Thread Andreas Rheinhardt
Jan Ekström:
> From: Jan Ekström 
> 
> Attempts to base the fragmentation timing on other streams
> as most receivers expect media fragments to be more or less
> aligned.
> 
> Currently does not support fragmentation on subtitle track
> only, as the subtitle packet queue timings would have to be
> checked in addition to the current fragmentation timing logic.
> 
> Signed-off-by: Jan Ekström 
> ---
>  libavformat/movenc.c|9 -
>  libavformat/movenc_ttml.c   |  157 ++-
>  tests/fate/mov.mak  |   21 +
>  tests/ref/fate/mov-mp4-fragmented-ttml-dfxp | 1197 +++
>  tests/ref/fate/mov-mp4-fragmented-ttml-stpp | 1197 +++

Am I the only one who thinks that this is a bit excessive?

>  5 files changed, 2568 insertions(+), 13 deletions(-)
>  create mode 100644 tests/ref/fate/mov-mp4-fragmented-ttml-dfxp
>  create mode 100644 tests/ref/fate/mov-mp4-fragmented-ttml-stpp
> 
> diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak
> index 6cb493ceab..5c44299196 100644
> --- a/tests/fate/mov.mak
> +++ b/tests/fate/mov.mak
> @@ -143,6 +143,27 @@ FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML SUBRIP, 
> MP4 MOV, SRT_DEMUXER TTML
>  fate-mov-mp4-ttml-stpp: CMD = transcode srt 
> $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 "-map 0:s -c:s ttml 
> -time_base:s 1:1000" "-map 0 -c copy" "-of json -show_entries 
> packet:stream=index,codec_type,codec_tag_string,codec_tag,codec_name,time_base,start_time,duration_ts,duration,nb_frames,nb_read_packets:stream_tags"
>  fate-mov-mp4-ttml-dfxp: CMD = transcode srt 
> $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 "-map 0:s -c:s ttml 
> -time_base:s 1:1000 -tag:s dfxp -strict unofficial" "-map 0 -c copy" "-of 
> json -show_entries 
> packet:stream=index,codec_type,codec_tag_string,codec_tag,codec_name,time_base,start_time,duration_ts,duration,nb_frames,nb_read_packets:stream_tags"
>  
> +FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML SUBRIP, MP4 MOV, LAVFI_INDEV 
> SMPTEHDBARS_FILTER SRT_DEMUXER MPEG2VIDEO_ENCODER TTML_MUXER RAWVIDEO_MUXER) 
> += fate-mov-mp4-fragmented-ttml-stpp
> +fate-mov-mp4-fragmented-ttml-stpp: CMD = transcode srt 
> $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 \
> +  "-map 1:v -map 0:s \
> +   -c:v mpeg2video -b:v 2M -g 48 -sc_threshold 10 \
> +   -c:s ttml -time_base:s 1:1000 \
> +   -movflags +cmaf" \
> +  "-map 0:s -c copy" \
> +  "-select_streams s -of csv -show_packets -show_data_hash crc32" \
> +  "-f lavfi -i 
> smptehdbars=duration=70:size=320x180:rate=24000/1001,format=yuv420p" \
> +  "" "" "rawvideo"

Would it speed the test up if you used smaller dimensions or a smaller
bitrate?
Anyway, you probably want the "data" output format instead of rawvideo.

> +
> +FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML SUBRIP, ISMV MOV, LAVFI_INDEV 
> SMPTEHDBARS_FILTER SRT_DEMUXER MPEG2VIDEO_ENCODER TTML_MUXER RAWVIDEO_MUXER) 
> += fate-mov-mp4-fragmented-ttml-dfxp
> +fate-mov-mp4-fragmented-ttml-dfxp: CMD = transcode srt 
> $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt ismv \
> +  "-map 1:v -map 0:s \
> +   -c:v mpeg2video -b:v 2M -g 48 -sc_threshold 10 \
> +   -c:s ttml -tag:s dfxp -time_base:s 1:1000" \
> +  "-map 0:s -c copy" \
> +  "-select_streams s -of csv -show_packets -show_data_hash crc32" \
> +  "-f lavfi -i 
> smptehdbars=duration=70:size=320x180:rate=24000/1001,format=yuv420p" \
> +  "" "" "rawvideo"
> +
>  # FIXME: Uncomment these two tests once the test files are uploaded to the 
> fate
>  # server.
>  # avif demuxing - still image with 1 item.


___
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] lavc/libopenh264: Drop openh264 runtime version checks

2023-12-08 Thread Martin Storsjö

On Fri, 8 Dec 2023, Kalev Lember wrote:



On Fri, Dec 8, 2023 at 1:00 PM Martin Storsjö  wrote:
  On Fri, 8 Dec 2023, Kalev Lember wrote:

  > As for dlopening, I think instead of version checks, it would
  make sense to
  > try to dlsym() all of the actual required symbols, and error
  out in init if
  > anything is missing. That should make it all super flexible
  and resilient to
  > e.g. struct size changes that would normally be an ABI change.

  How would that help, if e.g. the SEncParamExt struct in
  svc_encode_init
  would change layout/size - which part would notice that change?


Ah, hm, I didn't think this through apparently :) This would indeed still be
an issue.

I guess maybe dlopening the soname version that matches the headers (e.g.
libopenh264.so.7) would work then? With the expectation that upstream bumps
soname whenever the struct layout/size changes.


Yeah I guess that would work, it's all up to who has the responsibility 
for keeping it in sync. At some point, I envisioned that one could run it 
with e.g. -openh264_lib /path/to/my/libopenh264.so, and in such a 
scenario, we definitely would need some sort of reassurance that we're 
using the right ABI.


But anyway, good that we agree how this works. And this wasn't relevant 
for our current way of linking it here anyway, so the patch still is ok 
(and can be pushed later if nobody else has further opinions on it).


// Martin
___
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 v6 14/14] vvcdec: add full vvc decoder

2023-12-08 Thread Andreas Rheinhardt
Nuo Mi:
> vvc decoder plug-in to avcodec.
> split frames into slices/tiles and send them to vvc_thread for further 
> decoding
> reorder and wait for the frame decoding to be done and output the frame
> 
> Features:
> + Support I, P, B frames
> + Support 8/10/12 bits, chroma 400, 420, 422, and 444 and range extension
> + Support VVC new tools like MIP, CCLM, AFFINE, GPM, DMVR, PROF, BDOF, 
> LMCS, ALF
> + 295 conformace clips passed
> - Not support RPR, IBC, PALETTE, and other minor features yet
> 
> Performance:
> C code FPS on i7-12700 (x86):
> BQTerrace_1920x1080_60_10_420_22_RA.vvc  93.0
> Chimera_8bit_1080P_1000_frames.vvc  184.3
> NovosobornayaSquare_1920x1080.bin   191.3
> RitualDance_1920x1080_60_10_420_32_LD.266   150.7
> RitualDance_1920x1080_60_10_420_37_RA.266   170.0
> Tango2_3840x2160_60_10_420_27_LD.266 33.7
> 
> C code FPS on M1 Mac Pro (ARM):
> BQTerrace_1920x1080_60_10_420_22_RA.vvc 58.7
> Chimera_8bit_1080P_1000_frames.vvc  153.3
> NovosobornayaSquare_1920x1080.bin   150.3
> RitualDance_1920x1080_60_10_420_32_LD.266   105.0
> RitualDance_1920x1080_60_10_420_37_RA.266   133.0
> Tango2_3840x2160_60_10_420_27_LD.26621.7
> 
> Asm optimizations still working in progress. please check
> https://github.com/ffvvc/FFmpeg/wiki#performance-data for the latest
> 
> Contributors(based on code merge order):
> Nuo Mi 
> Xu Mu 
> frankplow 
> Shaun Loo 
> ---
>  libavcodec/vvc/vvcdec.c | 1007 +++
>  1 file changed, 1007 insertions(+)
> 
> diff --git a/libavcodec/vvc/vvcdec.c b/libavcodec/vvc/vvcdec.c
> index 3c591ce875..e40eb7339f 100644
> --- a/libavcodec/vvc/vvcdec.c
> +++ b/libavcodec/vvc/vvcdec.c
> @@ -21,28 +21,1035 @@
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 
> USA
>   */
>  #include "libavcodec/codec_internal.h"
> +#include "libavcodec/decode.h"
>  #include "libavcodec/profiles.h"
> +#include "libavcodec/refstruct.h"
> +#include "libavutil/cpu.h"
>  
>  #include "vvcdec.h"
> +#include "vvc_ctu.h"
> +#include "vvc_data.h"
> +#include "vvc_refs.h"
> +#include "vvc_thread.h"
> +
> +static int vvc_frame_start(VVCContext *s, VVCFrameContext *fc, SliceContext 
> *sc)
> +{
> +const VVCPH *ph = &fc->ps.ph;
> +const H266RawSliceHeader *rsh   = sc->sh.r;
> +int ret;
> +
> +// 8.3.1 Decoding process for picture order count
> +if (!s->temporal_id && !ph->r->ph_non_ref_pic_flag && !(IS_RASL(s) || 
> IS_RADL(s)))
> +s->poc_tid0 = ph->poc;
> +
> +if ((ret = ff_vvc_set_new_ref(s, fc, &fc->frame)) < 0)
> +goto fail;
> +
> +if (!IS_IDR(s))
> +ff_vvc_bump_frame(s, fc);
> +
> +av_frame_unref(fc->output_frame);
> +
> +if ((ret = ff_vvc_output_frame(s, fc, 
> fc->output_frame,rsh->sh_no_output_of_prior_pics_flag, 0)) < 0)
> +goto fail;
> +
> +if ((ret = ff_vvc_frame_rpl(s, fc, sc)) < 0)
> +goto fail;
> +
> +if ((ret = ff_vvc_frame_thread_init(fc)) < 0)
> +goto fail;
> +return 0;
> +fail:
> +if (fc->ref)
> +ff_vvc_unref_frame(fc, fc->ref, ~0);
> +fc->ref = NULL;
> +return ret;
> +}
> +
> +static void ctb_arrays_free(VVCFrameContext *fc)
> +{
> +av_freep(&fc->tab.deblock);
> +av_freep(&fc->tab.sao);
> +av_freep(&fc->tab.alf);
> +av_freep(&fc->tab.slice_idx);
> +av_freep(&fc->tab.coeffs);
> +if (fc->tab.ctus) {
> +for (int i = 0; i < fc->tab.ctu_count; i++)
> +ff_vvc_ctu_free_cus(fc->tab.ctus + i);
> +av_freep(&fc->tab.ctus);
> +}
> +ff_refstruct_pool_uninit(&fc->rpl_tab_pool);
> +}
> +
> +static int ctb_arrays_init(VVCFrameContext *fc, const int ctu_count, const 
> int ctu_size)
> +{
> +if (fc->tab.ctu_count != ctu_count || fc->tab.ctu_size != ctu_size) {
> +ctb_arrays_free(fc);
> +fc->tab.deblock = av_calloc(ctu_count, 
> sizeof(*fc->tab.deblock));
> +fc->tab.sao = av_calloc(ctu_count, sizeof(*fc->tab.sao));
> +fc->tab.alf = av_calloc(ctu_count, sizeof(*fc->tab.alf));
> +fc->tab.ctus= av_calloc(ctu_count, 
> sizeof(*fc->tab.ctus));
> +fc->tab.slice_idx   = av_malloc(ctu_count * 
> sizeof(*fc->tab.slice_idx));
> +if (!fc->tab.deblock || !fc->tab.sao || !fc->tab.alf || 
> !fc->tab.ctus || !fc->tab.slice_idx )
> +return AVERROR(ENOMEM);
> +fc->tab.coeffs = av_malloc(ctu_count * sizeof(*fc->tab.coeffs) * 
> ctu_size * VVC_MAX_SAMPLE_ARRAYS);
> +if (!fc->tab.coeffs)
> +return AVERROR(ENOMEM);
> +fc->rpl_tab_pool = ff_refstruct_pool_alloc(ctu_count * 
> sizeof(RefPicListTab), 0);
> +if (!fc->rpl_tab_pool)
> +return AVERROR(ENOMEM);
> +} else {
> +memset(fc->tab.deblock, 0, ctu_co

Re: [FFmpeg-devel] [PATCH v6 03/14] vvcdec: add parameter parser for sps, pps, ph, sh

2023-12-08 Thread Andreas Rheinhardt
Nuo Mi:
> ---
>  libavcodec/vvc/Makefile |1 +
>  libavcodec/vvc/vvc_ps.c | 1149 +++
>  libavcodec/vvc/vvc_ps.h |  263 +
>  libavcodec/vvc/vvcdec.h |7 +
>  4 files changed, 1420 insertions(+)
>  create mode 100644 libavcodec/vvc/vvc_ps.c
>  create mode 100644 libavcodec/vvc/vvc_ps.h
> 
> diff --git a/libavcodec/vvc/Makefile b/libavcodec/vvc/Makefile
> index 8a5c66ab13..b2b187ac6f 100644
> --- a/libavcodec/vvc/Makefile
> +++ b/libavcodec/vvc/Makefile
> @@ -3,3 +3,4 @@ clean::
>  
>  OBJS-$(CONFIG_VVC_DECODER)  +=  vvc/vvcdec.o\
>  vvc/vvc_data.o  \
> +vvc/vvc_ps.o\
> diff --git a/libavcodec/vvc/vvc_ps.c b/libavcodec/vvc/vvc_ps.c
> new file mode 100644
> index 00..284e7406c2
> --- /dev/null
> +++ b/libavcodec/vvc/vvc_ps.c
> @@ -0,0 +1,1149 @@
> +/*
> + * VVC parameter set parser
> + *
> + * Copyright (C) 2023 Nuo Mi
> + * Copyright (C) 2022 Xu Mu
> + *
> + * 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 "libavcodec/cbs_h266.h"
> +#include "libavutil/imgutils.h"
> +#include "libavcodec/refstruct.h"
> +#include "vvc_data.h"
> +#include "vvc_ps.h"
> +#include "vvcdec.h"
> +
> +static int sps_map_pixel_format(VVCSPS *sps, void *log_ctx)
> +{
> +const H266RawSPS *r = sps->r;
> +const AVPixFmtDescriptor *desc;
> +
> +switch (sps->bit_depth) {
> +case 8:
> +if (r->sps_chroma_format_idc == 0) sps->pix_fmt = AV_PIX_FMT_GRAY8;
> +if (r->sps_chroma_format_idc == 1) sps->pix_fmt = AV_PIX_FMT_YUV420P;
> +if (r->sps_chroma_format_idc == 2) sps->pix_fmt = AV_PIX_FMT_YUV422P;
> +if (r->sps_chroma_format_idc == 3) sps->pix_fmt = AV_PIX_FMT_YUV444P;
> +   break;
> +case 10:
> +if (r->sps_chroma_format_idc == 0) sps->pix_fmt = AV_PIX_FMT_GRAY10;
> +if (r->sps_chroma_format_idc == 1) sps->pix_fmt = 
> AV_PIX_FMT_YUV420P10;
> +if (r->sps_chroma_format_idc == 2) sps->pix_fmt = 
> AV_PIX_FMT_YUV422P10;
> +if (r->sps_chroma_format_idc == 3) sps->pix_fmt = 
> AV_PIX_FMT_YUV444P10;
> +break;
> +case 12:
> +if (r->sps_chroma_format_idc == 0) sps->pix_fmt = AV_PIX_FMT_GRAY12;
> +if (r->sps_chroma_format_idc == 1) sps->pix_fmt = 
> AV_PIX_FMT_YUV420P12;
> +if (r->sps_chroma_format_idc == 2) sps->pix_fmt = 
> AV_PIX_FMT_YUV422P12;
> +if (r->sps_chroma_format_idc == 3) sps->pix_fmt = 
> AV_PIX_FMT_YUV444P12;
> +break;
> +default:
> +av_log(log_ctx, AV_LOG_ERROR,
> +   "The following bit-depths are currently specified: 8, 10, 12 
> bits, "
> +   "chroma_format_idc is %d, depth is %d\n",
> +   r->sps_chroma_format_idc, sps->bit_depth);
> +return AVERROR_INVALIDDATA;
> +}
> +
> +desc = av_pix_fmt_desc_get(sps->pix_fmt);
> +if (!desc)
> +return AVERROR(EINVAL);
> +
> +sps->hshift[0] = sps->vshift[0] = 0;
> +sps->hshift[2] = sps->hshift[1] = desc->log2_chroma_w;
> +sps->vshift[2] = sps->vshift[1] = desc->log2_chroma_h;
> +
> +sps->pixel_shift = sps->bit_depth > 8;
> +
> +return 0;
> +}
> +
> +static int sps_bit_depth(VVCSPS *sps, void *log_ctx)
> +{
> +const H266RawSPS *r = sps->r;
> +
> +sps->bit_depth = r->sps_bitdepth_minus8 + 8;
> +sps->qp_bd_offset = 6 * (sps->bit_depth - 8);
> +sps->log2_transform_range =
> +r->sps_extended_precision_flag ? FFMAX(15, FFMIN(20, sps->bit_depth 
> + 6)) : 15;
> +return sps_map_pixel_format(sps, log_ctx);
> +}
> +
> +static int sps_chroma_qp_table(VVCSPS *sps)
> +{
> +const H266RawSPS *r = sps->r;
> +const int num_qp_tables = r->sps_same_qp_table_for_chroma_flag ?
> +1 : (r->sps_joint_cbcr_enabled_flag ? 3 : 2);
> +
> +for (int i = 0; i < num_qp_tables; i++) {
> +int num_points_in_qp_table;
> +int8_t qp_in[VVC_MAX_POINTS_IN_QP_TABLE], 
> qp_out[VVC_MAX_POINTS_IN_QP_TABLE];
> +unsigned int delta_qp_in[VVC_MAX_POINTS_IN_QP_TABLE];
> +int off = sps->qp_bd_offset;
> +
> +num_points_in_qp_table = r->sps_num_points_in_qp_table_minus1[i] + 1;
> +
> +qp_out[0

Re: [FFmpeg-devel] [PATCH v2 3/3] avformat/movenc: add support for fragmented TTML muxing

2023-12-08 Thread Dennis Mungai
On Fri, 8 Dec 2023 at 15:14, Andreas Rheinhardt <
andreas.rheinha...@outlook.com> wrote:

> Jan Ekström:
> > From: Jan Ekström 
> >
> > Attempts to base the fragmentation timing on other streams
> > as most receivers expect media fragments to be more or less
> > aligned.
> >
> > Currently does not support fragmentation on subtitle track
> > only, as the subtitle packet queue timings would have to be
> > checked in addition to the current fragmentation timing logic.
> >
> > Signed-off-by: Jan Ekström 
> > ---
> >  libavformat/movenc.c|9 -
> >  libavformat/movenc_ttml.c   |  157 ++-
> >  tests/fate/mov.mak  |   21 +
> >  tests/ref/fate/mov-mp4-fragmented-ttml-dfxp | 1197 +++
> >  tests/ref/fate/mov-mp4-fragmented-ttml-stpp | 1197 +++
>
> Am I the only one who thinks that this is a bit excessive?
>
> >  5 files changed, 2568 insertions(+), 13 deletions(-)
> >  create mode 100644 tests/ref/fate/mov-mp4-fragmented-ttml-dfxp
> >  create mode 100644 tests/ref/fate/mov-mp4-fragmented-ttml-stpp
> >
> > diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak
> > index 6cb493ceab..5c44299196 100644
> > --- a/tests/fate/mov.mak
> > +++ b/tests/fate/mov.mak
> > @@ -143,6 +143,27 @@ FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML
> SUBRIP, MP4 MOV, SRT_DEMUXER TTML
> >  fate-mov-mp4-ttml-stpp: CMD = transcode srt
> $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 "-map 0:s -c:s ttml
> -time_base:s 1:1000" "-map 0 -c copy" "-of json -show_entries
> packet:stream=index,codec_type,codec_tag_string,codec_tag,codec_name,time_base,start_time,duration_ts,duration,nb_frames,nb_read_packets:stream_tags"
> >  fate-mov-mp4-ttml-dfxp: CMD = transcode srt
> $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 "-map 0:s -c:s ttml
> -time_base:s 1:1000 -tag:s dfxp -strict unofficial" "-map 0 -c copy" "-of
> json -show_entries
> packet:stream=index,codec_type,codec_tag_string,codec_tag,codec_name,time_base,start_time,duration_ts,duration,nb_frames,nb_read_packets:stream_tags"
> >
> > +FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML SUBRIP, MP4 MOV,
> LAVFI_INDEV SMPTEHDBARS_FILTER SRT_DEMUXER MPEG2VIDEO_ENCODER TTML_MUXER
> RAWVIDEO_MUXER) += fate-mov-mp4-fragmented-ttml-stpp
> > +fate-mov-mp4-fragmented-ttml-stpp: CMD = transcode srt
> $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 \
> > +  "-map 1:v -map 0:s \
> > +   -c:v mpeg2video -b:v 2M -g 48 -sc_threshold 10 \
> > +   -c:s ttml -time_base:s 1:1000 \
> > +   -movflags +cmaf" \
> > +  "-map 0:s -c copy" \
> > +  "-select_streams s -of csv -show_packets -show_data_hash crc32" \
> > +  "-f lavfi -i
> smptehdbars=duration=70:size=320x180:rate=24000/1001,format=yuv420p" \
> > +  "" "" "rawvideo"
>
> Would it speed the test up if you used smaller dimensions or a smaller
> bitrate?
> Anyway, you probably want the "data" output format instead of rawvideo.
>
> > +
> > +FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML SUBRIP, ISMV MOV,
> LAVFI_INDEV SMPTEHDBARS_FILTER SRT_DEMUXER MPEG2VIDEO_ENCODER TTML_MUXER
> RAWVIDEO_MUXER) += fate-mov-mp4-fragmented-ttml-dfxp
> > +fate-mov-mp4-fragmented-ttml-dfxp: CMD = transcode srt
> $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt ismv \
> > +  "-map 1:v -map 0:s \
> > +   -c:v mpeg2video -b:v 2M -g 48 -sc_threshold 10 \
> > +   -c:s ttml -tag:s dfxp -time_base:s 1:1000" \
> > +  "-map 0:s -c copy" \
> > +  "-select_streams s -of csv -show_packets -show_data_hash crc32" \
> > +  "-f lavfi -i
> smptehdbars=duration=70:size=320x180:rate=24000/1001,format=yuv420p" \
> > +  "" "" "rawvideo"
> > +
> >  # FIXME: Uncomment these two tests once the test files are uploaded to
> the fate
> >  # server.
> >  # avif demuxing - still image with 1 item.
>

Hello Jan,

Taking this note into account, and I quote:

 " Currently does not support fragmentation on subtitle track only, as the
subtitle packet queue timings would have to be checked in addition to the
current fragmentation timing logic."

Wouldn't it be ideal to have this merged until after support for
fragmentation in subtitle-only tracks is complete, at the very least? That
way, the fate tests for such a workflow (case in point CMAF) would
therefore be feature complete?
The typical workloads that depend on such functionality, such as ingesting
CMFT require a subtitle-only stream be present in such a representation.

See:
1.
https://www.unified-streaming.com/blog/cmaf-conformance-is-this-really-cmaf
2. https://www.unified-streaming.com/blog/live-media-ingest-cmaf
___
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] lavc/libopenh264: Drop openh264 runtime version checks

2023-12-08 Thread Neal Gompa
On Fri, Dec 8, 2023 at 3:16 AM Kalev Lember  wrote:
>
> Years ago, openh264 releases often changed their ABI without changing
> the library soname. To avoid running into ABI issues, a version check
> was added to lavc libopenh264 code to error out at runtime in case the
> build time and runtime openh264 versions don't match.
>
> This should no longer be an issue with newer openh264 releases and we
> can drop the runtime version check and rely on upstream doing the right
> thing and bump the library soname if the ABI changes, similar to how
> other libraries are consumed in ffmpeg.
>
> Almost all major distributions now include openh264 and this means there
> are more eyes on ABI changes and issues are discovered and reported
> quickly. See e.g. https://github.com/cisco/openh264/issues/3564 where an
> ABI issue was quickly discovered and fixed.
>
> Relaxing the check allows downstream distributions to build ffmpeg
> against e.g. openh264 2.3.1 and ship an update to ABI-compatible
> openh264 2.4.0, without needing to coordinate a lock step update between
> ffmpeg and openh264 (which can be difficult if openh264 is distributed
> by Cisco and ffmpeg comes from the distro, such as is the case for
> Fedora).
>
> Signed-off-by: Kalev Lember 
> ---
>  libavcodec/libopenh264.c| 15 ---
>  libavcodec/libopenh264.h|  2 --
>  libavcodec/libopenh264dec.c |  4 
>  libavcodec/libopenh264enc.c |  4 
>  4 files changed, 25 deletions(-)
>
> diff --git a/libavcodec/libopenh264.c b/libavcodec/libopenh264.c
> index 0f6d28ed88..c80c85ea8b 100644
> --- a/libavcodec/libopenh264.c
> +++ b/libavcodec/libopenh264.c
> @@ -46,18 +46,3 @@ void ff_libopenh264_trace_callback(void *ctx, int level, 
> const char *msg)
>  int equiv_ffmpeg_log_level = libopenh264_to_ffmpeg_log_level(level);
>  av_log(ctx, equiv_ffmpeg_log_level, "%s\n", msg);
>  }
> -
> -int ff_libopenh264_check_version(void *logctx)
> -{
> -// Mingw GCC < 4.7 on x86_32 uses an incorrect/buggy ABI for the 
> WelsGetCodecVersion
> -// function (for functions returning larger structs), thus skip the 
> check in those
> -// configurations.
> -#if !defined(_WIN32) || !defined(__GNUC__) || !ARCH_X86_32 || 
> AV_GCC_VERSION_AT_LEAST(4, 7)
> -OpenH264Version libver = WelsGetCodecVersion();
> -if (memcmp(&libver, &g_stCodecVersion, sizeof(libver))) {
> -av_log(logctx, AV_LOG_ERROR, "Incorrect library version loaded\n");
> -return AVERROR(EINVAL);
> -}
> -#endif
> -return 0;
> -}
> diff --git a/libavcodec/libopenh264.h b/libavcodec/libopenh264.h
> index dbb9c5d429..0b462d6fdc 100644
> --- a/libavcodec/libopenh264.h
> +++ b/libavcodec/libopenh264.h
> @@ -34,6 +34,4 @@
>
>  void ff_libopenh264_trace_callback(void *ctx, int level, const char *msg);
>
> -int ff_libopenh264_check_version(void *logctx);
> -
>  #endif /* AVCODEC_LIBOPENH264_H */
> diff --git a/libavcodec/libopenh264dec.c b/libavcodec/libopenh264dec.c
> index 7d650ae03e..b6a9bba2dc 100644
> --- a/libavcodec/libopenh264dec.c
> +++ b/libavcodec/libopenh264dec.c
> @@ -52,13 +52,9 @@ static av_cold int svc_decode_init(AVCodecContext *avctx)
>  {
>  SVCContext *s = avctx->priv_data;
>  SDecodingParam param = { 0 };
> -int err;
>  int log_level;
>  WelsTraceCallback callback_function;
>
> -if ((err = ff_libopenh264_check_version(avctx)) < 0)
> -return AVERROR_DECODER_NOT_FOUND;
> -
>  if (WelsCreateDecoder(&s->decoder)) {
>  av_log(avctx, AV_LOG_ERROR, "Unable to create decoder\n");
>  return AVERROR_UNKNOWN;
> diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> index f518d0894e..6f231d22b2 100644
> --- a/libavcodec/libopenh264enc.c
> +++ b/libavcodec/libopenh264enc.c
> @@ -110,14 +110,10 @@ static av_cold int svc_encode_init(AVCodecContext 
> *avctx)
>  {
>  SVCContext *s = avctx->priv_data;
>  SEncParamExt param = { 0 };
> -int err;
>  int log_level;
>  WelsTraceCallback callback_function;
>  AVCPBProperties *props;
>
> -if ((err = ff_libopenh264_check_version(avctx)) < 0)
> -return AVERROR_ENCODER_NOT_FOUND;
> -
>  if (WelsCreateSVCEncoder(&s->encoder)) {
>  av_log(avctx, AV_LOG_ERROR, "Unable to create encoder\n");
>  return AVERROR_UNKNOWN;
> --
> 2.43.0
>

Thank you for this. It looks good to me.

Reviewed-by: Neal Gompa 



-- 
真実はいつも一つ!/ Always, there's only one truth!
___
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] lavc/libopenh264: Drop openh264 runtime version checks

2023-12-08 Thread James Almer

On 12/8/2023 5:15 AM, Kalev Lember wrote:

Years ago, openh264 releases often changed their ABI without changing
the library soname. To avoid running into ABI issues, a version check
was added to lavc libopenh264 code to error out at runtime in case the
build time and runtime openh264 versions don't match.

This should no longer be an issue with newer openh264 releases and we
can drop the runtime version check and rely on upstream doing the right
thing and bump the library soname if the ABI changes, similar to how
other libraries are consumed in ffmpeg.


Does the configure check ensure a new enough openh264 version is the 
minimum supported?




Almost all major distributions now include openh264 and this means there
are more eyes on ABI changes and issues are discovered and reported
quickly. See e.g. https://github.com/cisco/openh264/issues/3564 where an
ABI issue was quickly discovered and fixed.

Relaxing the check allows downstream distributions to build ffmpeg
against e.g. openh264 2.3.1 and ship an update to ABI-compatible
openh264 2.4.0, without needing to coordinate a lock step update between
ffmpeg and openh264 (which can be difficult if openh264 is distributed
by Cisco and ffmpeg comes from the distro, such as is the case for
Fedora).

Signed-off-by: Kalev Lember 
---
  libavcodec/libopenh264.c| 15 ---
  libavcodec/libopenh264.h|  2 --
  libavcodec/libopenh264dec.c |  4 
  libavcodec/libopenh264enc.c |  4 
  4 files changed, 25 deletions(-)

diff --git a/libavcodec/libopenh264.c b/libavcodec/libopenh264.c
index 0f6d28ed88..c80c85ea8b 100644
--- a/libavcodec/libopenh264.c
+++ b/libavcodec/libopenh264.c
@@ -46,18 +46,3 @@ void ff_libopenh264_trace_callback(void *ctx, int level, 
const char *msg)
  int equiv_ffmpeg_log_level = libopenh264_to_ffmpeg_log_level(level);
  av_log(ctx, equiv_ffmpeg_log_level, "%s\n", msg);
  }
-
-int ff_libopenh264_check_version(void *logctx)
-{
-// Mingw GCC < 4.7 on x86_32 uses an incorrect/buggy ABI for the 
WelsGetCodecVersion
-// function (for functions returning larger structs), thus skip the check 
in those
-// configurations.
-#if !defined(_WIN32) || !defined(__GNUC__) || !ARCH_X86_32 || 
AV_GCC_VERSION_AT_LEAST(4, 7)
-OpenH264Version libver = WelsGetCodecVersion();
-if (memcmp(&libver, &g_stCodecVersion, sizeof(libver))) {
-av_log(logctx, AV_LOG_ERROR, "Incorrect library version loaded\n");
-return AVERROR(EINVAL);
-}
-#endif
-return 0;
-}
diff --git a/libavcodec/libopenh264.h b/libavcodec/libopenh264.h
index dbb9c5d429..0b462d6fdc 100644
--- a/libavcodec/libopenh264.h
+++ b/libavcodec/libopenh264.h
@@ -34,6 +34,4 @@
  
  void ff_libopenh264_trace_callback(void *ctx, int level, const char *msg);
  
-int ff_libopenh264_check_version(void *logctx);

-
  #endif /* AVCODEC_LIBOPENH264_H */
diff --git a/libavcodec/libopenh264dec.c b/libavcodec/libopenh264dec.c
index 7d650ae03e..b6a9bba2dc 100644
--- a/libavcodec/libopenh264dec.c
+++ b/libavcodec/libopenh264dec.c
@@ -52,13 +52,9 @@ static av_cold int svc_decode_init(AVCodecContext *avctx)
  {
  SVCContext *s = avctx->priv_data;
  SDecodingParam param = { 0 };
-int err;
  int log_level;
  WelsTraceCallback callback_function;
  
-if ((err = ff_libopenh264_check_version(avctx)) < 0)

-return AVERROR_DECODER_NOT_FOUND;
-
  if (WelsCreateDecoder(&s->decoder)) {
  av_log(avctx, AV_LOG_ERROR, "Unable to create decoder\n");
  return AVERROR_UNKNOWN;
diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
index f518d0894e..6f231d22b2 100644
--- a/libavcodec/libopenh264enc.c
+++ b/libavcodec/libopenh264enc.c
@@ -110,14 +110,10 @@ static av_cold int svc_encode_init(AVCodecContext *avctx)
  {
  SVCContext *s = avctx->priv_data;
  SEncParamExt param = { 0 };
-int err;
  int log_level;
  WelsTraceCallback callback_function;
  AVCPBProperties *props;
  
-if ((err = ff_libopenh264_check_version(avctx)) < 0)

-return AVERROR_ENCODER_NOT_FOUND;
-
  if (WelsCreateSVCEncoder(&s->encoder)) {
  av_log(avctx, AV_LOG_ERROR, "Unable to create encoder\n");
  return AVERROR_UNKNOWN;

___
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 v6 14/14] vvcdec: add full vvc decoder

2023-12-08 Thread Nuo Mi
Hi Andreas,
thank you for the review.
On Fri, Dec 8, 2023 at 8:17 PM Andreas Rheinhardt <
andreas.rheinha...@outlook.com> wrote:

>
> > +
> > +static int min_pu_arrays_init(VVCFrameContext *fc, const int
> pic_size_in_min_pu)
> > +{
> > +if (fc->tab.pic_size_in_min_pu != pic_size_in_min_pu) {
> > +min_pu_arrays_free(fc);
> > +fc->tab.msf  = av_mallocz(pic_size_in_min_pu);
> > +fc->tab.iaf  = av_mallocz(pic_size_in_min_pu);
> > +fc->tab.mmi  = av_mallocz(pic_size_in_min_pu);
> > +fc->tab.mvf  = av_mallocz(pic_size_in_min_pu *
> sizeof(*fc->tab.mvf));
>
> Do these have to be separate allocations? If there were allocated
> jointly, one memset below would suffice.
>
They are separate flags, if we combine them. We can't use memset to set
flags for a block.

>
> > +
> > +if (!fc->cu_pool) {
> > +fc->cu_pool = ff_refstruct_pool_alloc(sizeof(CodingUnit), 0);
> > +if (!fc->cu_pool)
> > +goto fail;
>
> The size of the objects contained in this pool don't depend on any
> bitstream parameters. You can therefore simply use a single pool (in
> VVCContext) that is allocated in vvc_decode_init() and freed in
> vvc_decode_free().
> The same goes for tu_pool below.
>
A global pool may have a performance issue for huge thread number.
Move it to frame_context_init

>
>
>
> > +static int slices_realloc(VVCFrameContext *fc)
> > +{
> > +void *p;
> > +const int size = (fc->nb_slices_allocated + 1) * 3 / 2;
> > +
> > +if (fc->nb_slices < fc->nb_slices_allocated)
> > +return 0;
> > +
> > +p = av_realloc(fc->slices, size * sizeof(*fc->slices));
>
> av_realloc_array()
>
 done

>
> > +if (!p)
> > +return AVERROR(ENOMEM);
> > +
> > +fc->slices = p;
> > +for (int i = fc->nb_slices_allocated; i < size; i++) {
> > +fc->slices[i] = av_calloc(1, sizeof(*fc->slices[0]));
>
> av_mallocz().
>
done

>
> > +if (!fc->slices[i]) {
> > +for (int j = fc->nb_slices_allocated; j < i; j++)
> > +av_freep(&fc->slices[j]);
> > +return AVERROR(ENOMEM);
>
> Can't you simply set fc->nb_slices_allocated to i in order to avoid this
> loop?
>
done

> > +
> > +static int init_slice_context(SliceContext *sc, VVCFrameContext *fc,
> const H2645NAL *nal, const CodedBitstreamUnit *unit)
> > +{
> > +const VVCSH *sh = &sc->sh;
> > +const H266RawSlice *slice   = (const H266RawSlice *)unit->content;
>
> Please no pointless casts. Also, why is there unnecessary whitespace in
> front of '='?
>
Fix here and serval other places
The whitespace will make all = in a col.


> > +int nb_eps  = sh->r->num_entry_points + 1;
> > +int ctu_addr= 0;
> > +GetBitContext gb;
> > +
> > +if (sc->nb_eps != nb_eps) {
> > +eps_free(sc);
> > +sc->eps = av_calloc(nb_eps, sizeof(*sc->eps));
> > +if (!sc->eps)
> > +return AVERROR(ENOMEM);
>
> In case of error, sc->eps is NULL, yet sc->nb_eps may be != 0. Stuff
> like this can (and does) lead to crashes.
>
added "slice->nb_eps = 0;" to eps_free

>
> > +static int vvc_ref_frame(VVCFrameContext *fc, VVCFrame *dst, VVCFrame
> *src)
>
> src should be const.
>
done

>
> > +
> > +static av_cold int frame_context_init(VVCFrameContext *fc,
> AVCodecContext *avctx)
> > +{
> > +
> > +fc->avctx = av_memdup(avctx, sizeof(*avctx));
>
> When I read this, I presumed you are using multiple AVCodecContexts to
> store the ever changing state of the AVCodecContext fields similarly to
> update_context_from_thread() in pthread_frame.c. But it seems you don't.
> These contexts are only used as a) logcontexts (where the actual
> user-facing AVCodecContext should be used, so that the user can make
> sense of the logmessages!), b) in ff_thread_get_buffer() and c) in
> export_frame_params() where only some basic fields
> (dimension-related+pix_fmt) is set. Presumably c) is done for b).
>
I remember if i did not use a local AVCodecContext  it would trigger some
assert when resolution changed.

>
> But the user is allowed to change the provided callbacks in the master
> context at any time. E.g. the call to ff_thread_get_buffer() in
> vvc_refs.c currently uses the VVCFrameContext and therefore uses the
> get_buffer2 callback in place now (during av_memdup()). This is wrong.
>
This will not happen. av_memdup only happens in vvc_decode_init.
Nobody will call ff_thread_get_buffer at this time

>
> I think you can just remove VVCFrameContext.avctx and use the
> user-facing AVCodecContext if you set the AVFrame properties that are
> normally derived from the AVCodecContext directly on the AVFrame before
> ff_thread_get_buffer().

Could you explain more about how to create a user-facing  AVCodecContext?

>
> > +
> > +static int decode_nal_unit(VVCContext *s, VVCFrameContext *fc, const
> H2645NAL *nal, const CodedBitstreamUnit *unit)
> > +{
> > +int  ret;
> > +
> > +s->temporal_id   = nal->tempor

Re: [FFmpeg-devel] [PATCH v6 03/14] vvcdec: add parameter parser for sps, pps, ph, sh

2023-12-08 Thread Nuo Mi
Hi Andreas,
Thank you for the review.

On Fri, Dec 8, 2023 at 8:25 PM Andreas Rheinhardt <
andreas.rheinha...@outlook.com> wrote:

>
>
> Missing allocation check.
>
done.

>
>
> We know that sps and ps->sps_list[sps_id] don't coincide, so the latter
> will always be unreferenced. So you might just as well use one of these:
>
> ff_refstruct_unref(&ps->sps_list[sps_id]);
> ps->sps_list[sps_id] = sps;
>
done for sps and pps.

>
>
___
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] [MXF] - Add jpeg2000 subdescriptor in MXF file.

2023-12-08 Thread Cédric Le Barz

Add jpeg2000 subdescriptor in MXF file.
--- Begin Message ---
Signed-off-by: Cedric Le Barz 
---
 ffmpeg/libavformat/mxf.h|   1 +
 ffmpeg/libavformat/mxfenc.c | 173 +++-
 2 files changed, 173 insertions(+), 1 deletion(-)

diff --git a/ffmpeg/libavformat/mxf.h b/ffmpeg/libavformat/mxf.h
index 2561605..7dd1681 100644
--- a/ffmpeg/libavformat/mxf.h
+++ b/ffmpeg/libavformat/mxf.h
@@ -55,6 +55,7 @@ enum MXFMetadataSetType {
 SoundfieldGroupLabelSubDescriptor,
 GroupOfSoundfieldGroupsLabelSubDescriptor,
 FFV1SubDescriptor,
+JPEG2000SubDescriptor,
 };
 
 enum MXFFrameLayout {
diff --git a/ffmpeg/libavformat/mxfenc.c b/ffmpeg/libavformat/mxfenc.c
index 53bd6ae..a06c8af 100644
--- a/ffmpeg/libavformat/mxfenc.c
+++ b/ffmpeg/libavformat/mxfenc.c
@@ -48,8 +48,10 @@
 #include "libavutil/pixdesc.h"
 #include "libavutil/time_internal.h"
 #include "libavcodec/defs.h"
+#include "libavcodec/bytestream.h"
 #include "libavcodec/golomb.h"
 #include "libavcodec/h264.h"
+#include "libavcodec/jpeg2000.h"
 #include "libavcodec/packet_internal.h"
 #include "libavcodec/rangecoder.h"
 #include "libavcodec/startcode.h"
@@ -78,6 +80,20 @@ typedef struct MXFIndexEntry {
 uint8_t flags;
 } MXFIndexEntry;
 
+typedef struct j2k_info_t {
+uint16_t j2k_cap;///< j2k required decoder capabilities
+uint16_t j2k_rsiz;   ///< j2k required decoder capabilities (Rsiz)
+uint32_t j2k_xsiz;   ///< j2k width of the reference grid (Xsiz)
+uint32_t j2k_ysiz;   ///< j2k height of the reference grid (Ysiz)
+uint32_t j2k_x0siz;  ///< j2k horizontal offset from the origin of the 
reference grid to the left side of the image (X0siz)
+uint32_t j2k_y0siz;  ///< j2k vertical offset from the origin of the 
reference grid to the left side of the image (Y0siz)
+uint32_t j2k_xtsiz;  ///< j2k width of one reference tile with respect 
to the reference grid (XTsiz)
+uint32_t j2k_ytsiz;  ///< j2k height of one reference tile with 
respect to the reference grid (YTsiz)
+uint32_t j2k_xt0siz; ///< j2k horizontal offset from the origin of the 
reference grid to the left side of the first tile (XT0siz)
+uint32_t j2k_yt0siz; ///< j2k vertical offset from the origin of the 
reference grid to the left side of the first tile (YT0siz)
+uint8_t  j2k_comp_desc[12]; ///< j2k components descriptor (Ssiz(i), 
XRsiz(i), YRsiz(i))
+} j2k_info_t;
+
 typedef struct MXFStreamContext {
 int64_t pkt_cnt; ///< pkt counter for muxed packets
 UID track_essence_element_key;
@@ -104,6 +120,7 @@ typedef struct MXFStreamContext {
 int low_delay;   ///< low delay, used in mpeg-2 descriptor
 int avc_intra;
 int micro_version;   ///< format micro_version, used in ffv1 descriptor
+j2k_info_t j2k_info;
 } MXFStreamContext;
 
 typedef struct MXFContainerEssenceEntry {
@@ -413,6 +430,20 @@ static const MXFLocalTagPair mxf_local_tag_batch[] = {
 { 0xDFD9, 
{0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x0E,0x04,0x01,0x06,0x0C,0x06,0x00,0x00,0x00}},
 /* FFV1 Micro-version */
 { 0xDFDA, 
{0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x0E,0x04,0x01,0x06,0x0C,0x05,0x00,0x00,0x00}},
 /* FFV1 Version */
 { 0xDFDB, 
{0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x0E,0x04,0x01,0x06,0x0C,0x01,0x00,0x00,0x00}},
 /* FFV1 Initialization Metadata */
+// ff_mxf_jpeg2000_local_tags
+{ 0x8400, 
{0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x09,0x06,0x01,0x01,0x04,0x06,0x10,0x00,0x00}},
 /* Sub Descriptors / Opt Ordered array of strong references to sub descriptor 
sets */
+{ 0x8401, 
{0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x01,0x00,0x00,0x00}},
 /* Rsiz: An enumerated value that defines the decoder capabilities */
+{ 0x8402, 
{0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x02,0x00,0x00,0x00}},
 /* Xsiz: Width of the reference grid */
+{ 0x8403, 
{0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x03,0x00,0x00,0x00}},
 /* Ysiz: Height of the reference grid */
+{ 0x8404, 
{0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x04,0x00,0x00,0x00}},
 /* X0siz: Horizontal offset from the origin of the reference grid to the left 
side of the image area */
+{ 0x8405, 
{0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x05,0x00,0x00,0x00}},
 /* Y0siz: Vertical offset from the origin of the reference grid to the left 
side of the image area */
+{ 0x8406, 
{0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x06,0x00,0x00,0x00}},
 /* XTsiz: Width of one reference tile with respect to the reference grid */
+{ 0x8407, 
{0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x07,0x00,0x00,0x00}},
 /* YTsiz: Height of one reference tile with respect to the reference grid */
+{ 0x8408, 
{0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x08,0x00,0x00,0x00}},
 /* XT0siz: Horizontal offset from the origin of the reference grid to the left 
side of th

Re: [FFmpeg-devel] [PATCH v5 00/14] encoder AVCodecContext configuration side data

2023-12-08 Thread James Almer

On 12/8/2023 8:59 AM, Jan Ekström wrote:

On Sun, Nov 26, 2023 at 9:58 PM Jan Ekström  wrote:


Differences to v3:
1. rebased on top of current master
2. moved the addition of multiple side data entries from a generic
av_frame_side_data_set_extend to avcodec as per request from James.
4. adopted various things noted by reviews

Comparison URL (mostly configure and wrappers, avutil/frame.c):
https://github.com/jeeb/ffmpeg/compare/avcodec_cll_mdcv_side_data_v4..avcodec_cll_mdcv_side_data_v5

This patch set I've now been working for a while since I felt like it was weird
we couldn't pass through information such as static HDR metadata to encoders
from decoded input. This initial version adds the necessary framework, as well
as adds static HDR metadata support for libsvtav1, libx264 as well as libx265
wrappers.

An alternative to this would be to make encoders only properly initialize when
they receive the first AVFrame, but that seems to be a bigger, nastier change
than introducing an AVFrameSideDataSet in avctx as everything seems to
presume that extradata etc are available after opening the encoder.

Note: Any opinions on whether FFCodec or AVCodec should have
   handled_side_data list, so that if format specific generic logic is
   added, it could be checked whether the codec itself handles this side
   data? This could also be utilized to list handled side data from f.ex.
   `ffmpeg -h encoder=libsvtav1`.

Jan


Ping.

I'd like to understand whether:

* people are fine with the struct which lets you pass things as a
single argument, or they would like to move all the helper functions
to counter and pointer.


IMO we should do the same we did for packet side data, so IN/OUT pointer 
and size as arguments. It should also match the function signatures as 
much as possible. So for example:



AVFrameSideData *av_frame_side_data_new(AVPacketSideData ***sd, int *nb_sd,
enum AVFrameSideDataType type,
size_t size, int flags);



AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd,
enum AVFrameSideDataType type,
void *data, size_t size, int flags);



AVFrameSideData *av_packet_side_data_add_from_buf(AVFrameSideData ***sd, int 
*nb_sd,
  enum AVFrameSideDataType type,
 /* making a new ref, not taking ownership */ AVBufferRef *buf, int flags);



const AVFrameSideData *av_frame_side_data_get(const AVFrameSideData **sd,
  int nb_sd,
  enum AVFrameSideDataType type);



void av_frame_side_data_remove(AVFrameSideData ***sd, int *nb_sd,
   enum AVFrameSideDataType type);



void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);



const char *av_frame_side_data_name(enum AVFrameSideDataType type);


And since unlike with packet we allow more than one entry per frame side 
data type, you could add something like:



const AVFrameSideData *av_frame_side_data_iterate(const AVFrameSideData **sd,
  int nb_sd,
  enum AVPacketSideDataType 
type,
  void **opaque);



* should the avcodec helper take in the struct/{counter,pointer}, or
should it instead take in a const AVFrame pointer?


The former. There already are helpers for side data in AVFrame, and the 
point of this set is to add such side data to AVCodecContext.




as I'd like to start pulling this in, and move towards working on
implemented side data listing in AVCodecs, as well as adding generic
implementations for specific codecs (such as H.264/HEVC/AV1 having CBS
create side data generically, without the need of specific AVCodecs
implementing things),

Jan
___
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 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 v2 3/3] avformat/movenc: add support for fragmented TTML muxing

2023-12-08 Thread Jan Ekström
On Fri, Dec 8, 2023 at 5:37 PM Dennis Mungai  wrote:
>
> On Fri, 8 Dec 2023 at 15:14, Andreas Rheinhardt <
> andreas.rheinha...@outlook.com> wrote:
>
> > Jan Ekström:
> > > From: Jan Ekström 
> > >
> > > Attempts to base the fragmentation timing on other streams
> > > as most receivers expect media fragments to be more or less
> > > aligned.
> > >
> > > Currently does not support fragmentation on subtitle track
> > > only, as the subtitle packet queue timings would have to be
> > > checked in addition to the current fragmentation timing logic.
> > >
> > > Signed-off-by: Jan Ekström 
> > > ---
> > >  libavformat/movenc.c|9 -
> > >  libavformat/movenc_ttml.c   |  157 ++-
> > >  tests/fate/mov.mak  |   21 +
> > >  tests/ref/fate/mov-mp4-fragmented-ttml-dfxp | 1197 +++
> > >  tests/ref/fate/mov-mp4-fragmented-ttml-stpp | 1197 +++
> >
> > Am I the only one who thinks that this is a bit excessive?
> >
> > >  5 files changed, 2568 insertions(+), 13 deletions(-)
> > >  create mode 100644 tests/ref/fate/mov-mp4-fragmented-ttml-dfxp
> > >  create mode 100644 tests/ref/fate/mov-mp4-fragmented-ttml-stpp
> > >
> > > diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak
> > > index 6cb493ceab..5c44299196 100644
> > > --- a/tests/fate/mov.mak
> > > +++ b/tests/fate/mov.mak
> > > @@ -143,6 +143,27 @@ FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML
> > SUBRIP, MP4 MOV, SRT_DEMUXER TTML
> > >  fate-mov-mp4-ttml-stpp: CMD = transcode srt
> > $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 "-map 0:s -c:s ttml
> > -time_base:s 1:1000" "-map 0 -c copy" "-of json -show_entries
> > packet:stream=index,codec_type,codec_tag_string,codec_tag,codec_name,time_base,start_time,duration_ts,duration,nb_frames,nb_read_packets:stream_tags"
> > >  fate-mov-mp4-ttml-dfxp: CMD = transcode srt
> > $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 "-map 0:s -c:s ttml
> > -time_base:s 1:1000 -tag:s dfxp -strict unofficial" "-map 0 -c copy" "-of
> > json -show_entries
> > packet:stream=index,codec_type,codec_tag_string,codec_tag,codec_name,time_base,start_time,duration_ts,duration,nb_frames,nb_read_packets:stream_tags"
> > >
> > > +FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML SUBRIP, MP4 MOV,
> > LAVFI_INDEV SMPTEHDBARS_FILTER SRT_DEMUXER MPEG2VIDEO_ENCODER TTML_MUXER
> > RAWVIDEO_MUXER) += fate-mov-mp4-fragmented-ttml-stpp
> > > +fate-mov-mp4-fragmented-ttml-stpp: CMD = transcode srt
> > $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 \
> > > +  "-map 1:v -map 0:s \
> > > +   -c:v mpeg2video -b:v 2M -g 48 -sc_threshold 10 \
> > > +   -c:s ttml -time_base:s 1:1000 \
> > > +   -movflags +cmaf" \
> > > +  "-map 0:s -c copy" \
> > > +  "-select_streams s -of csv -show_packets -show_data_hash crc32" \
> > > +  "-f lavfi -i
> > smptehdbars=duration=70:size=320x180:rate=24000/1001,format=yuv420p" \
> > > +  "" "" "rawvideo"
> >
> > Would it speed the test up if you used smaller dimensions or a smaller
> > bitrate?
> > Anyway, you probably want the "data" output format instead of rawvideo.
> >
> > > +
> > > +FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML SUBRIP, ISMV MOV,
> > LAVFI_INDEV SMPTEHDBARS_FILTER SRT_DEMUXER MPEG2VIDEO_ENCODER TTML_MUXER
> > RAWVIDEO_MUXER) += fate-mov-mp4-fragmented-ttml-dfxp
> > > +fate-mov-mp4-fragmented-ttml-dfxp: CMD = transcode srt
> > $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt ismv \
> > > +  "-map 1:v -map 0:s \
> > > +   -c:v mpeg2video -b:v 2M -g 48 -sc_threshold 10 \
> > > +   -c:s ttml -tag:s dfxp -time_base:s 1:1000" \
> > > +  "-map 0:s -c copy" \
> > > +  "-select_streams s -of csv -show_packets -show_data_hash crc32" \
> > > +  "-f lavfi -i
> > smptehdbars=duration=70:size=320x180:rate=24000/1001,format=yuv420p" \
> > > +  "" "" "rawvideo"
> > > +
> > >  # FIXME: Uncomment these two tests once the test files are uploaded to
> > the fate
> > >  # server.
> > >  # avif demuxing - still image with 1 item.
> >
>
> Hello Jan,
>
> Taking this note into account, and I quote:
>
>  " Currently does not support fragmentation on subtitle track only, as the
> subtitle packet queue timings would have to be checked in addition to the
> current fragmentation timing logic."
>
> Wouldn't it be ideal to have this merged until after support for
> fragmentation in subtitle-only tracks is complete, at the very least? That
> way, the fate tests for such a workflow (case in point CMAF) would
> therefore be feature complete?
> The typical workloads that depend on such functionality, such as ingesting
> CMFT require a subtitle-only stream be present in such a representation.
>
> See:
> 1.
> https://www.unified-streaming.com/blog/cmaf-conformance-is-this-really-cmaf
> 2. https://www.unified-streaming.com/blog/live-media-ingest-cmaf

It would be ideal, but there are a few points to keep in mind:

1. For such streaming, you are generally required to be synchronized
in your fragmentation against othe

Re: [FFmpeg-devel] [PATCH v2 3/3] avformat/movenc: add support for fragmented TTML muxing

2023-12-08 Thread Jan Ekström
On Fri, Dec 8, 2023 at 7:05 PM Jan Ekström  wrote:
>
> On Fri, Dec 8, 2023 at 5:37 PM Dennis Mungai  wrote:
> >
> > On Fri, 8 Dec 2023 at 15:14, Andreas Rheinhardt <
> > andreas.rheinha...@outlook.com> wrote:
> >
> > > Jan Ekström:
> > > > From: Jan Ekström 
> > > >
> > > > Attempts to base the fragmentation timing on other streams
> > > > as most receivers expect media fragments to be more or less
> > > > aligned.
> > > >
> > > > Currently does not support fragmentation on subtitle track
> > > > only, as the subtitle packet queue timings would have to be
> > > > checked in addition to the current fragmentation timing logic.
> > > >
> > > > Signed-off-by: Jan Ekström 
> > > > ---
> > > >  libavformat/movenc.c|9 -
> > > >  libavformat/movenc_ttml.c   |  157 ++-
> > > >  tests/fate/mov.mak  |   21 +
> > > >  tests/ref/fate/mov-mp4-fragmented-ttml-dfxp | 1197 +++
> > > >  tests/ref/fate/mov-mp4-fragmented-ttml-stpp | 1197 +++
> > >
> > > Am I the only one who thinks that this is a bit excessive?
> > >
> > > >  5 files changed, 2568 insertions(+), 13 deletions(-)
> > > >  create mode 100644 tests/ref/fate/mov-mp4-fragmented-ttml-dfxp
> > > >  create mode 100644 tests/ref/fate/mov-mp4-fragmented-ttml-stpp
> > > >
> > > > diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak
> > > > index 6cb493ceab..5c44299196 100644
> > > > --- a/tests/fate/mov.mak
> > > > +++ b/tests/fate/mov.mak
> > > > @@ -143,6 +143,27 @@ FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML
> > > SUBRIP, MP4 MOV, SRT_DEMUXER TTML
> > > >  fate-mov-mp4-ttml-stpp: CMD = transcode srt
> > > $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 "-map 0:s -c:s ttml
> > > -time_base:s 1:1000" "-map 0 -c copy" "-of json -show_entries
> > > packet:stream=index,codec_type,codec_tag_string,codec_tag,codec_name,time_base,start_time,duration_ts,duration,nb_frames,nb_read_packets:stream_tags"
> > > >  fate-mov-mp4-ttml-dfxp: CMD = transcode srt
> > > $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 "-map 0:s -c:s ttml
> > > -time_base:s 1:1000 -tag:s dfxp -strict unofficial" "-map 0 -c copy" "-of
> > > json -show_entries
> > > packet:stream=index,codec_type,codec_tag_string,codec_tag,codec_name,time_base,start_time,duration_ts,duration,nb_frames,nb_read_packets:stream_tags"
> > > >
> > > > +FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML SUBRIP, MP4 MOV,
> > > LAVFI_INDEV SMPTEHDBARS_FILTER SRT_DEMUXER MPEG2VIDEO_ENCODER TTML_MUXER
> > > RAWVIDEO_MUXER) += fate-mov-mp4-fragmented-ttml-stpp
> > > > +fate-mov-mp4-fragmented-ttml-stpp: CMD = transcode srt
> > > $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 \
> > > > +  "-map 1:v -map 0:s \
> > > > +   -c:v mpeg2video -b:v 2M -g 48 -sc_threshold 10 \
> > > > +   -c:s ttml -time_base:s 1:1000 \
> > > > +   -movflags +cmaf" \
> > > > +  "-map 0:s -c copy" \
> > > > +  "-select_streams s -of csv -show_packets -show_data_hash crc32" \
> > > > +  "-f lavfi -i
> > > smptehdbars=duration=70:size=320x180:rate=24000/1001,format=yuv420p" \
> > > > +  "" "" "rawvideo"
> > >
> > > Would it speed the test up if you used smaller dimensions or a smaller
> > > bitrate?
> > > Anyway, you probably want the "data" output format instead of rawvideo.
> > >
> > > > +
> > > > +FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML SUBRIP, ISMV MOV,
> > > LAVFI_INDEV SMPTEHDBARS_FILTER SRT_DEMUXER MPEG2VIDEO_ENCODER TTML_MUXER
> > > RAWVIDEO_MUXER) += fate-mov-mp4-fragmented-ttml-dfxp
> > > > +fate-mov-mp4-fragmented-ttml-dfxp: CMD = transcode srt
> > > $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt ismv \
> > > > +  "-map 1:v -map 0:s \
> > > > +   -c:v mpeg2video -b:v 2M -g 48 -sc_threshold 10 \
> > > > +   -c:s ttml -tag:s dfxp -time_base:s 1:1000" \
> > > > +  "-map 0:s -c copy" \
> > > > +  "-select_streams s -of csv -show_packets -show_data_hash crc32" \
> > > > +  "-f lavfi -i
> > > smptehdbars=duration=70:size=320x180:rate=24000/1001,format=yuv420p" \
> > > > +  "" "" "rawvideo"
> > > > +
> > > >  # FIXME: Uncomment these two tests once the test files are uploaded to
> > > the fate
> > > >  # server.
> > > >  # avif demuxing - still image with 1 item.
> > >
> >
> > Hello Jan,
> >
> > Taking this note into account, and I quote:
> >
> >  " Currently does not support fragmentation on subtitle track only, as the
> > subtitle packet queue timings would have to be checked in addition to the
> > current fragmentation timing logic."
> >
> > Wouldn't it be ideal to have this merged until after support for
> > fragmentation in subtitle-only tracks is complete, at the very least? That
> > way, the fate tests for such a workflow (case in point CMAF) would
> > therefore be feature complete?
> > The typical workloads that depend on such functionality, such as ingesting
> > CMFT require a subtitle-only stream be present in such a representation.
> >
> > See:
> > 1.
> > https://www.unified-streaming.com/blog/cmaf-confor

Re: [FFmpeg-devel] [PATCH v2 3/3] avformat/movenc: add support for fragmented TTML muxing

2023-12-08 Thread Jan Ekström
On Fri, Dec 8, 2023 at 7:09 PM Jan Ekström  wrote:
>
> On Fri, Dec 8, 2023 at 7:05 PM Jan Ekström  wrote:
> >
> > On Fri, Dec 8, 2023 at 5:37 PM Dennis Mungai  wrote:
> > >
> > > On Fri, 8 Dec 2023 at 15:14, Andreas Rheinhardt <
> > > andreas.rheinha...@outlook.com> wrote:
> > >
> > > > Jan Ekström:
> > > > > From: Jan Ekström 
> > > > >
> > > > > Attempts to base the fragmentation timing on other streams
> > > > > as most receivers expect media fragments to be more or less
> > > > > aligned.
> > > > >
> > > > > Currently does not support fragmentation on subtitle track
> > > > > only, as the subtitle packet queue timings would have to be
> > > > > checked in addition to the current fragmentation timing logic.
> > > > >
> > > > > Signed-off-by: Jan Ekström 
> > > > > ---
> > > > >  libavformat/movenc.c|9 -
> > > > >  libavformat/movenc_ttml.c   |  157 ++-
> > > > >  tests/fate/mov.mak  |   21 +
> > > > >  tests/ref/fate/mov-mp4-fragmented-ttml-dfxp | 1197 
> > > > > +++
> > > > >  tests/ref/fate/mov-mp4-fragmented-ttml-stpp | 1197 
> > > > > +++
> > > >
> > > > Am I the only one who thinks that this is a bit excessive?
> > > >
> > > > >  5 files changed, 2568 insertions(+), 13 deletions(-)
> > > > >  create mode 100644 tests/ref/fate/mov-mp4-fragmented-ttml-dfxp
> > > > >  create mode 100644 tests/ref/fate/mov-mp4-fragmented-ttml-stpp
> > > > >
> > > > > diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak
> > > > > index 6cb493ceab..5c44299196 100644
> > > > > --- a/tests/fate/mov.mak
> > > > > +++ b/tests/fate/mov.mak
> > > > > @@ -143,6 +143,27 @@ FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML
> > > > SUBRIP, MP4 MOV, SRT_DEMUXER TTML
> > > > >  fate-mov-mp4-ttml-stpp: CMD = transcode srt
> > > > $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 "-map 0:s -c:s 
> > > > ttml
> > > > -time_base:s 1:1000" "-map 0 -c copy" "-of json -show_entries
> > > > packet:stream=index,codec_type,codec_tag_string,codec_tag,codec_name,time_base,start_time,duration_ts,duration,nb_frames,nb_read_packets:stream_tags"
> > > > >  fate-mov-mp4-ttml-dfxp: CMD = transcode srt
> > > > $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 "-map 0:s -c:s 
> > > > ttml
> > > > -time_base:s 1:1000 -tag:s dfxp -strict unofficial" "-map 0 -c copy" 
> > > > "-of
> > > > json -show_entries
> > > > packet:stream=index,codec_type,codec_tag_string,codec_tag,codec_name,time_base,start_time,duration_ts,duration,nb_frames,nb_read_packets:stream_tags"
> > > > >
> > > > > +FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML SUBRIP, MP4 MOV,
> > > > LAVFI_INDEV SMPTEHDBARS_FILTER SRT_DEMUXER MPEG2VIDEO_ENCODER TTML_MUXER
> > > > RAWVIDEO_MUXER) += fate-mov-mp4-fragmented-ttml-stpp
> > > > > +fate-mov-mp4-fragmented-ttml-stpp: CMD = transcode srt
> > > > $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt mp4 \
> > > > > +  "-map 1:v -map 0:s \
> > > > > +   -c:v mpeg2video -b:v 2M -g 48 -sc_threshold 10 \
> > > > > +   -c:s ttml -time_base:s 1:1000 \
> > > > > +   -movflags +cmaf" \
> > > > > +  "-map 0:s -c copy" \
> > > > > +  "-select_streams s -of csv -show_packets -show_data_hash crc32" \
> > > > > +  "-f lavfi -i
> > > > smptehdbars=duration=70:size=320x180:rate=24000/1001,format=yuv420p" \
> > > > > +  "" "" "rawvideo"
> > > >
> > > > Would it speed the test up if you used smaller dimensions or a smaller
> > > > bitrate?
> > > > Anyway, you probably want the "data" output format instead of rawvideo.
> > > >
> > > > > +
> > > > > +FATE_MOV_FFMPEG_FFPROBE-$(call TRANSCODE, TTML SUBRIP, ISMV MOV,
> > > > LAVFI_INDEV SMPTEHDBARS_FILTER SRT_DEMUXER MPEG2VIDEO_ENCODER TTML_MUXER
> > > > RAWVIDEO_MUXER) += fate-mov-mp4-fragmented-ttml-dfxp
> > > > > +fate-mov-mp4-fragmented-ttml-dfxp: CMD = transcode srt
> > > > $(TARGET_SAMPLES)/sub/SubRip_capability_tester.srt ismv \
> > > > > +  "-map 1:v -map 0:s \
> > > > > +   -c:v mpeg2video -b:v 2M -g 48 -sc_threshold 10 \
> > > > > +   -c:s ttml -tag:s dfxp -time_base:s 1:1000" \
> > > > > +  "-map 0:s -c copy" \
> > > > > +  "-select_streams s -of csv -show_packets -show_data_hash crc32" \
> > > > > +  "-f lavfi -i
> > > > smptehdbars=duration=70:size=320x180:rate=24000/1001,format=yuv420p" \
> > > > > +  "" "" "rawvideo"
> > > > > +
> > > > >  # FIXME: Uncomment these two tests once the test files are uploaded 
> > > > > to
> > > > the fate
> > > > >  # server.
> > > > >  # avif demuxing - still image with 1 item.
> > > >
> > >
> > > Hello Jan,
> > >
> > > Taking this note into account, and I quote:
> > >
> > >  " Currently does not support fragmentation on subtitle track only, as the
> > > subtitle packet queue timings would have to be checked in addition to the
> > > current fragmentation timing logic."
> > >
> > > Wouldn't it be ideal to have this merged until after support for
> > > fragmentation in subtitle-only tracks is complete, at the very least? That
> > > way, the fate tests f

[FFmpeg-devel] [PATCH] avcodec/libjxldec: emit proper PTS to decoded AVFrame

2023-12-08 Thread Leo Izen
If a sequence of JXL images is encapsulated in a container that has PTS
information, we should use the PTS information from the container. At
this time there is no container that does this, but if JPEG XL support
is ever added to NUT, AVTransport, or some other container, this commit
should allow the PTS information those containers provide to work as
expected.

Signed-off-by: Leo Izen 
---
 libavcodec/libjxldec.c | 77 +++---
 1 file changed, 57 insertions(+), 20 deletions(-)

diff --git a/libavcodec/libjxldec.c b/libavcodec/libjxldec.c
index 002740d9c1..494060ac8c 100644
--- a/libavcodec/libjxldec.c
+++ b/libavcodec/libjxldec.c
@@ -370,6 +370,7 @@ static int libjxl_receive_frame(AVCodecContext *avctx, 
AVFrame *frame)
 
 while (1) {
 size_t remaining;
+JxlFrameHeader header;
 
 if (!pkt->size) {
 av_packet_unref(pkt);
@@ -428,13 +429,16 @@ static int libjxl_receive_frame(AVCodecContext *avctx, 
AVFrame *frame)
 }
 if ((ret = ff_set_dimensions(avctx, ctx->basic_info.xsize, 
ctx->basic_info.ysize)) < 0)
 return ret;
+/*
+ * If animation is present, we use the timebase provided by
+ *the animated image itself.
+ * If the image is not animated, we use ctx->pts
+ *to refer to the frame number, not an actual
+ *PTS value, thus we may leave ctx->timebase unset.
+ */
 if (ctx->basic_info.have_animation)
 ctx->timebase = 
av_make_q(ctx->basic_info.animation.tps_denominator,
   
ctx->basic_info.animation.tps_numerator);
-else if (avctx->pkt_timebase.num)
-ctx->timebase = avctx->pkt_timebase;
-else
-ctx->timebase = AV_TIME_BASE_Q;
 continue;
 case JXL_DEC_COLOR_ENCODING:
 av_log(avctx, AV_LOG_DEBUG, "COLOR_ENCODING event emitted\n");
@@ -462,23 +466,24 @@ static int libjxl_receive_frame(AVCodecContext *avctx, 
AVFrame *frame)
 #endif
 continue;
 case JXL_DEC_FRAME:
+/* Frame here refers to the Frame bundle, not a decoded picture */
 av_log(avctx, AV_LOG_DEBUG, "FRAME event emitted\n");
-if (!ctx->basic_info.have_animation || ctx->prev_is_last) {
+if (ctx->prev_is_last) {
+/*
+ * The last frame sent was tagged as "is_last" which
+ * means this is a new image file altogether.
+ */
 ctx->frame->pict_type = AV_PICTURE_TYPE_I;
 ctx->frame->flags |= AV_FRAME_FLAG_KEY;
 }
-if (ctx->basic_info.have_animation) {
-JxlFrameHeader header;
-if (JxlDecoderGetFrameHeader(ctx->decoder, &header) != 
JXL_DEC_SUCCESS) {
-av_log(avctx, AV_LOG_ERROR, "Bad libjxl dec frame 
event\n");
-return AVERROR_EXTERNAL;
-}
-ctx->prev_is_last = header.is_last;
-ctx->frame_duration = header.duration;
-} else {
-ctx->prev_is_last = 1;
-ctx->frame_duration = 1;
+if (JxlDecoderGetFrameHeader(ctx->decoder, &header) != 
JXL_DEC_SUCCESS) {
+av_log(avctx, AV_LOG_ERROR, "Bad libjxl dec frame event\n");
+return AVERROR_EXTERNAL;
 }
+ctx->prev_is_last = header.is_last;
+/* zero duration for animation means the frame is not presented */
+if (ctx->basic_info.have_animation && header.duration)
+ctx->frame_duration = header.duration;
 continue;
 case JXL_DEC_FULL_IMAGE:
 /* full image is one frame, even if animated */
@@ -490,12 +495,44 @@ static int libjxl_receive_frame(AVCodecContext *avctx, 
AVFrame *frame)
 /* ownership is transfered, and it is not ref-ed */
 ctx->iccp = NULL;
 }
-if (avctx->pkt_timebase.num) {
-ctx->frame->pts = av_rescale_q(ctx->pts, ctx->timebase, 
avctx->pkt_timebase);
-ctx->frame->duration = av_rescale_q(ctx->frame_duration, 
ctx->timebase, avctx->pkt_timebase);
+if (ctx->basic_info.have_animation) {
+if (avctx->pkt_timebase.num) {
+/*
+ * ideally, the demuxer set avctx->pkt_timebase to equal 
the animation's timebase
+ * or something strictly finer. This is true about the 
jpegxl_anim demuxer.
+ */
+ctx->frame->pts = av_rescale_q(ctx->pts, ctx->timebase, 
avctx->pkt_timebase);
+ctx->frame->duration = av_rescale_q(ctx->frame_duration, 
ctx->timebase, avctx->pkt_timebase);
+} else {
+/*
+ * If we don't know t

[FFmpeg-devel] [PATCH] lavc/lpc: R-V V apply_welch_window

2023-12-08 Thread Rémi Denis-Courmont
apply_welch_window_even_c:   617.5
apply_welch_window_even_rvv_f64: 235.0
apply_welch_window_odd_c:709.0
apply_welch_window_odd_rvv_f64:  256.5
---
 libavcodec/lpc.c|  4 +-
 libavcodec/lpc.h|  1 +
 libavcodec/riscv/Makefile   |  2 +
 libavcodec/riscv/lpc_init.c | 37 
 libavcodec/riscv/lpc_rvv.S  | 88 +
 5 files changed, 131 insertions(+), 1 deletion(-)
 create mode 100644 libavcodec/riscv/lpc_init.c
 create mode 100644 libavcodec/riscv/lpc_rvv.S

diff --git a/libavcodec/lpc.c b/libavcodec/lpc.c
index dc6a3060ce..9e2fd0f128 100644
--- a/libavcodec/lpc.c
+++ b/libavcodec/lpc.c
@@ -320,7 +320,9 @@ av_cold int ff_lpc_init(LPCContext *s, int blocksize, int 
max_order,
 s->lpc_apply_welch_window = lpc_apply_welch_window_c;
 s->lpc_compute_autocorr   = lpc_compute_autocorr_c;
 
-#if ARCH_X86
+#if ARCH_RISCV
+ff_lpc_init_riscv(s);
+#elif ARCH_X86
 ff_lpc_init_x86(s);
 #endif
 
diff --git a/libavcodec/lpc.h b/libavcodec/lpc.h
index 467d0b2830..0200baea5c 100644
--- a/libavcodec/lpc.h
+++ b/libavcodec/lpc.h
@@ -109,6 +109,7 @@ double ff_lpc_calc_ref_coefs_f(LPCContext *s, const float 
*samples, int len,
  */
 int ff_lpc_init(LPCContext *s, int blocksize, int max_order,
 enum FFLPCType lpc_type);
+void ff_lpc_init_riscv(LPCContext *s);
 void ff_lpc_init_x86(LPCContext *s);
 
 /**
diff --git a/libavcodec/riscv/Makefile b/libavcodec/riscv/Makefile
index e9825c0856..1d4572fbc5 100644
--- a/libavcodec/riscv/Makefile
+++ b/libavcodec/riscv/Makefile
@@ -33,6 +33,8 @@ OBJS-$(CONFIG_LLVIDDSP) += riscv/llviddsp_init.o
 RVV-OBJS-$(CONFIG_LLVIDDSP) += riscv/llviddsp_rvv.o
 OBJS-$(CONFIG_LLVIDENCDSP) += riscv/llvidencdsp_init.o
 RVV-OBJS-$(CONFIG_LLVIDENCDSP) += riscv/llvidencdsp_rvv.o
+OBJS-$(CONFIG_LPC) += riscv/lpc_init.o
+RVV-OBJS-$(CONFIG_LPC) += riscv/lpc_rvv.o
 OBJS-$(CONFIG_OPUS_DECODER) += riscv/opusdsp_init.o
 RVV-OBJS-$(CONFIG_OPUS_DECODER) += riscv/opusdsp_rvv.o
 OBJS-$(CONFIG_PIXBLOCKDSP) += riscv/pixblockdsp_init.o
diff --git a/libavcodec/riscv/lpc_init.c b/libavcodec/riscv/lpc_init.c
new file mode 100644
index 00..c16e5745f0
--- /dev/null
+++ b/libavcodec/riscv/lpc_init.c
@@ -0,0 +1,37 @@
+/*
+ * Copyright © 2022 Rémi Denis-Courmont.
+ *
+ * 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 "config.h"
+
+#include "libavutil/attributes.h"
+#include "libavutil/cpu.h"
+#include "libavcodec/lpc.h"
+
+void ff_lpc_apply_welch_window_rvv(const int32_t *, ptrdiff_t, double *);
+
+av_cold void ff_lpc_init_riscv(LPCContext *c)
+{
+#if HAVE_RVV && (__riscv_xlen >= 64)
+int flags = av_get_cpu_flags();
+
+if ((flags & AV_CPU_FLAG_RVV_F64) && (flags & AV_CPU_FLAG_RVB_ADDR))
+c->lpc_apply_welch_window = ff_lpc_apply_welch_window_rvv;
+#endif
+}
diff --git a/libavcodec/riscv/lpc_rvv.S b/libavcodec/riscv/lpc_rvv.S
new file mode 100644
index 00..2bc729d400
--- /dev/null
+++ b/libavcodec/riscv/lpc_rvv.S
@@ -0,0 +1,88 @@
+/*
+ * Copyright © 2023 Rémi Denis-Courmont.
+ *
+ * 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/riscv/asm.S"
+
+#if __riscv_xlen >= 64
+func ff_lpc_apply_welch_window_rvv, zve64d
+vsetvli t0, zero, e64, m8, ta, ma
+vid.v   v0
+addit2, a1, -1
+vfcvt.f.xu.v v0, v0
+li  t3, 2
+fcvt.d.l ft2, t2
+srait1, a1, 1
+fcvt.d.l ft3, t3
+li  t4, 1
+fdiv.d  ft0, ft3, ft2# ft0 = c = 2. / (len - 1)
+fcvt.d.l fa1, t4 

Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes

2023-12-08 Thread Timo Rothenpieler

On 08.12.2023 11:01, Andreas Rheinhardt wrote:

Timo Rothenpieler:

FFmpeg has instances of DECLARE_ALIGNED(32, ...) in a lot of structs,
which then end up heap-allocated.
By declaring any variable in a struct, or tree of structs, to be 32 byte
aligned, it allows the compiler to safely assume the entire struct
itself is also 32 byte aligned.

This might make the compiler emit code which straight up crashes or
misbehaves in other ways, and at least in one instances is now
documented to actually do (see ticket 10549 on trac).
The issue there is that an unrelated variable in SingleChannelElement is
declared to have an alignment of 32 bytes. So if the compiler does a copy
in decode_cpe() with avx instructions, but ffmpeg is built with
--disable-avx, this results in a crash, since the memory is only 16 byte
aligned.
Mind you, even if the compiler does not emit avx instructions, the code
is still invalid and could misbehave. It just happens not to. Declaring
any variable in a struct with a 32 byte alignment promises 32 byte
alignment of the whole struct to the compiler.

Instead of now going through all instances of variables in structs
being declared as 32 byte aligned, this patch bumps the minimum alignment
to 32 bytes.
---
  libavutil/mem.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavutil/mem.c b/libavutil/mem.c
index 36b8940a0c..26a9b9753b 100644
--- a/libavutil/mem.c
+++ b/libavutil/mem.c
@@ -62,7 +62,7 @@ void  free(void *ptr);
  
  #endif /* MALLOC_PREFIX */
  
-#define ALIGN (HAVE_AVX512 ? 64 : (HAVE_AVX ? 32 : 16))

+#define ALIGN (HAVE_AVX512 ? 64 : 32)
  
  /* NOTE: if you want to override these functions with your own

   * implementations (not recommended) you have to link libav* as


1. There is another way in which this can be triggered: Namely if one
uses a build with AVX, but combines it with a lavu built without it; it
is also triggerable on non-x86 (having an insufficiently aligned pointer
is always UB even if the CPU does not have instructions that would
benefit from the additional alignment). You should mention this in the
commit message.


Is mixing the libraries really a scenario we need to care about/support?

And yeah, this is only marginally related to AVX, in that it's what 
triggers a crash in this scenario.
But as stated in the commit message, it's not a valid thing to do in any 
case, on any arch. It just happens not to crash.



2. This topic gave me headaches when creating RefStruct. I "solved" it
by (ab)using STRIDE_ALIGN which mimicks the alignment of av_malloc(),
thereby ensuring that RefStruct does not break lavc builds built with
the avx dsp functions enabled (but it does not guard against using a
lavu whose av_malloc() only provides less alignment).

3. There is a downside to your patch: It bumps alignment for non-x86
arches which wastes memory (and may make allocators slower). We could
fix this by modifying the 32-byte-alignment macros to only provide 16
byte alignment if the ARCH_ (and potentially the HAVE_) defines indicate
that no alignment bigger than 16 is needed.


But it's invalid on any other arch as well, just hasn't bitten us yet.
I'm not sure if I'd want to start maintaining a growingly complex list 
of CPU extensions and make the DECLARE_ALIGNED macro lie if we think it 
doesn't need 32 byte alignment.

We don't really know why someone wants a variable aligned after all.



- 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-requ...@ffmpeg.org with subject "unsubscribe".

___
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] avutil/mem: always align by at least 32 bytes

2023-12-08 Thread Nicolas George
Timo Rothenpieler (12023-12-08):
> Is mixing the libraries really a scenario we need to care about/support?

No. We should merge all the libraries into a single libffmpeg.so. Having
separate libraries brings us no end of hassle and drawbacks, starting
with all the avpriv symbols and backward compatibility layers, and the
benefits it brings could be reached in simpler and more efficient ways.

But anytime I brought it up, the same naysayers would object, but when I
ask what precise benefit they think the current situation brings, with
the intent of explaining how it can be done better differently (I do not
bring that half-backed, I have thought about it beforehand) or in some
case explaining that no, this is not a benefit because linking does not
work like that. And then the naysayers would whine that I am making too
much a fuss.

Barring merging all libraries into a single libffmpeg.so, have configure
compute AV_LIBRARY_SIGNATURE as a 64 bits hash of the version and
configuration, then have in version.h of each library:

#define avsmth_version_check_signature() \
  avsmth_version_check_signature_ ## AV_LIBRARY_SIGNATURE()

then have avsmth_version_check_signature() in each library call the ones
in the library it depends on, and core functions like *register_all()
call it. Then if the libraries are mixed at run time it will produce an
error message by the linker that can be searched on the web.

Regards,

-- 
  Nicolas George


signature.asc
Description: PGP signature
___
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] avfilter/vf_libvmaf: fix string comparison bug

2023-12-08 Thread Kyle Swanson
On Wed, Dec 6, 2023 at 9:54 AM Kyle Swanson  wrote:
> Will push this tomorrow.

Pushed, thanks.
___
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] doc/filters: expand documentation on libvmaf filter

2023-12-08 Thread Nil Fons Miret via ffmpeg-devel
Attached.


0001-doc-filters-expand-documentation-on-libvmaf-filter.patch
Description: Binary data
___
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] lavc/libopenh264: Drop openh264 runtime version checks

2023-12-08 Thread Kalev Lember
On Fri, Dec 8, 2023 at 4:59 PM James Almer  wrote:

> Does the configure check ensure a new enough openh264 version is the
> minimum supported?
>

Hm, I'd say that configure minimum version check is mostly orthogonal to
the patch here.

This patch just removes a check that made it error out if the build time
and runtime versions didn't perfectly match (as in, 1.0.0 at build time and
1.0.1 at run time would have resulted in erroring out). Basically just
makes it behave like with all other libraries :)

-- 
Kalev
___
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] lavc/libopenh264: Drop openh264 runtime version checks

2023-12-08 Thread James Almer

On 12/8/2023 4:07 PM, Kalev Lember wrote:

On Fri, Dec 8, 2023 at 4:59 PM James Almer  wrote:


Does the configure check ensure a new enough openh264 version is the
minimum supported?



Hm, I'd say that configure minimum version check is mostly orthogonal to
the patch here.

This patch just removes a check that made it error out if the build time
and runtime versions didn't perfectly match (as in, 1.0.0 at build time and
1.0.1 at run time would have resulted in erroring out). Basically just
makes it behave like with all other libraries :)


I understand that, but you said "This should no longer be an issue with 
newer openh264 releases", meaning this used to be a problem until the 
project started being more careful about breakages, so shouldn't we bump 
the minimum required version to one where it's know there will be no 
issues at runtime if the runtime library is ABI incompatible with the 
link time one?

___
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] lavc/libopenh264: Drop openh264 runtime version checks

2023-12-08 Thread Cosmin Stejerean via ffmpeg-devel


> On Dec 8, 2023, at 11:07 AM, Kalev Lember  wrote:
> 
> On Fri, Dec 8, 2023 at 4:59 PM James Almer  wrote:
> 
>> Does the configure check ensure a new enough openh264 version is the
>> minimum supported?
>> 
> 
> Hm, I'd say that configure minimum version check is mostly orthogonal to
> the patch here.
> 
> This patch just removes a check that made it error out if the build time
> and runtime versions didn't perfectly match (as in, 1.0.0 at build time and
> 1.0.1 at run time would have resulted in erroring out). Basically just
> makes it behave like with all other libraries :)

As of what version of openh264 though is it safe to assume that ABI won't 
change without soname changes? Since years ago ABI changes without soname 
changes were present there's likely to be some minimum version above which 
runtime version checks are not needed.

- Cosmin
___
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] lavc/libopenh264: Drop openh264 runtime version checks

2023-12-08 Thread Kalev Lember
On Fri, Dec 8, 2023 at 8:12 PM Cosmin Stejerean via ffmpeg-devel <
ffmpeg-devel@ffmpeg.org> wrote:

> As of what version of openh264 though is it safe to assume that ABI won't
> change without soname changes? Since years ago ABI changes without soname
> changes were present there's likely to be some minimum version above which
> runtime version checks are not needed.
>

I don't have a very good answer here, sorry. It was more of a general
observation that upstream is trying to keep the soname updated whenever
there is an ABI change.

However, if I had to pick a version, I did some digging and maybe version
1.3.0 because before that there was just an unversioned libopenh264.so, and
1.3.0 added an actual versioned libopenh264.so.0, which has been updated
since then whenever there have been ABI changes.

Do you guys want me to add a minimum 1.3.0 check?

Martin mentioned earlier that he once envisioned something like passing
-openh264_lib /path/to/my/libopenh264.so to ffmpeg and that must have been
before the versioned soname was introduced.

-- 
Kalev
___
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] lavc/libopenh264: Drop openh264 runtime version checks

2023-12-08 Thread Martin Storsjö

On Fri, 8 Dec 2023, Kalev Lember wrote:


On Fri, Dec 8, 2023 at 8:12 PM Cosmin Stejerean via ffmpeg-devel <
ffmpeg-devel@ffmpeg.org> wrote:


As of what version of openh264 though is it safe to assume that ABI won't
change without soname changes? Since years ago ABI changes without soname
changes were present there's likely to be some minimum version above which
runtime version checks are not needed.



I don't have a very good answer here, sorry. It was more of a general
observation that upstream is trying to keep the soname updated whenever
there is an ABI change.

However, if I had to pick a version, I did some digging and maybe version
1.3.0 because before that there was just an unversioned libopenh264.so, and
1.3.0 added an actual versioned libopenh264.so.0, which has been updated
since then whenever there have been ABI changes.

Do you guys want me to add a minimum 1.3.0 check?


If that was when the soname version was introduced, that sounds reasonable 
- since then, there's at least an intent not to break it (even if mistakes 
always can happen).



Martin mentioned earlier that he once envisioned something like passing
-openh264_lib /path/to/my/libopenh264.so to ffmpeg and that must have been
before the versioned soname was introduced.


Not necessarily; my point was that if we would have allowed pointing at a 
specific file, we need to check that it matches the expected ABI version. 
As we don't have an ABI version number in the headers (and there was no 
effort to maintain an ABI at the time), checking the full version number 
was my approximation of it.


// Martin
___
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 v6 14/14] vvcdec: add full vvc decoder

2023-12-08 Thread Andreas Rheinhardt
Nuo Mi:
> Hi Andreas,
> thank you for the review.
> On Fri, Dec 8, 2023 at 8:17 PM Andreas Rheinhardt <
> andreas.rheinha...@outlook.com> wrote:
> 
>>
>>> +
>>> +static int min_pu_arrays_init(VVCFrameContext *fc, const int
>> pic_size_in_min_pu)
>>> +{
>>> +if (fc->tab.pic_size_in_min_pu != pic_size_in_min_pu) {
>>> +min_pu_arrays_free(fc);
>>> +fc->tab.msf  = av_mallocz(pic_size_in_min_pu);
>>> +fc->tab.iaf  = av_mallocz(pic_size_in_min_pu);
>>> +fc->tab.mmi  = av_mallocz(pic_size_in_min_pu);
>>> +fc->tab.mvf  = av_mallocz(pic_size_in_min_pu *
>> sizeof(*fc->tab.mvf));
>>
>> Do these have to be separate allocations? If there were allocated
>> jointly, one memset below would suffice.
>>
> They are separate flags, if we combine them. We can't use memset to set
> flags for a block.
> 

I disagree: You would still be able to use different pointers for
different parts of the large allocated block, it is just that you also
save some unnecessary allocations (and frees and errors checks for the
allocations) and also gain the ability to memset them via one memset
call in case one wants to set them to the same value.

>>
>>> +
>>> +static int init_slice_context(SliceContext *sc, VVCFrameContext *fc,
>> const H2645NAL *nal, const CodedBitstreamUnit *unit)
>>> +{
>>> +const VVCSH *sh = &sc->sh;
>>> +const H266RawSlice *slice   = (const H266RawSlice *)unit->content;
>>
>> Please no pointless casts. Also, why is there unnecessary whitespace in
>> front of '='?
>>
> Fix here and serval other places
> The whitespace will make all = in a col.
> 

But there is nothing that needs that much whitespace.

>>> +
>>> +static av_cold int frame_context_init(VVCFrameContext *fc,
>> AVCodecContext *avctx)
>>> +{
>>> +
>>> +fc->avctx = av_memdup(avctx, sizeof(*avctx));
>>
>> When I read this, I presumed you are using multiple AVCodecContexts to
>> store the ever changing state of the AVCodecContext fields similarly to
>> update_context_from_thread() in pthread_frame.c. But it seems you don't.
>> These contexts are only used as a) logcontexts (where the actual
>> user-facing AVCodecContext should be used, so that the user can make
>> sense of the logmessages!), b) in ff_thread_get_buffer() and c) in
>> export_frame_params() where only some basic fields
>> (dimension-related+pix_fmt) is set. Presumably c) is done for b).
>>
> I remember if i did not use a local AVCodecContext  it would trigger some
> assert when resolution changed.
> 

Can you be more specific about what assert has been triggered? And have
you set the AVFrame fields directly before ff_thread_get_buffer()?

>>
>> But the user is allowed to change the provided callbacks in the master
>> context at any time. E.g. the call to ff_thread_get_buffer() in
>> vvc_refs.c currently uses the VVCFrameContext and therefore uses the
>> get_buffer2 callback in place now (during av_memdup()). This is wrong.
>>
> This will not happen. av_memdup only happens in vvc_decode_init.
> Nobody will call ff_thread_get_buffer at this time
> 

You missed the point: If the user changes the get_buffer2 callback after
init, the new callback will not be used at all.

>>
>> I think you can just remove VVCFrameContext.avctx and use the
>> user-facing AVCodecContext if you set the AVFrame properties that are
>> normally derived from the AVCodecContext directly on the AVFrame before
>> ff_thread_get_buffer().
> 
> Could you explain more about how to create a user-facing  AVCodecContext?
> 

You do not create a user-facing AVCodecContext, the user does (and calls
avcodec_send_packet()/avcodec_receive_frame() with it).

>>
>>> +
>>> +static int decode_nal_unit(VVCContext *s, VVCFrameContext *fc, const
>> H2645NAL *nal, const CodedBitstreamUnit *unit)
>>> +{
>>> +int  ret;
>>> +
>>> +s->temporal_id   = nal->temporal_id;
>>> +
>>> +switch (unit->type) {
>>> +case VVC_VPS_NUT:
>>> +case VVC_SPS_NUT:
>>> +case VVC_PPS_NUT:
>>> +/* vps, sps, sps cached by s->cbc */
>>> +break;
>>> +case VVC_TRAIL_NUT:
>>> +case VVC_STSA_NUT:
>>> +case VVC_RADL_NUT:
>>> +case VVC_RASL_NUT:
>>> +case VVC_IDR_W_RADL:
>>> +case VVC_IDR_N_LP:
>>> +case VVC_CRA_NUT:
>>> +case VVC_GDR_NUT:
>>> +ret = decode_slice(s, fc, nal, unit);
>>> +if (ret < 0)
>>> +goto fail;
>>> +break;
>>> +case VVC_PREFIX_APS_NUT:
>>> +case VVC_SUFFIX_APS_NUT:
>>> +ret = ff_vvc_decode_aps(&s->ps, unit);
>>> +if (ret < 0)
>>> +goto fail;
>>> +break;
>>> +default:
>>> +av_log(s->avctx, AV_LOG_INFO,
>>> +   "Skipping NAL unit %d\n", unit->type);
>>
>> This will probably be very noisy (and warn for every SEI). I don't think
>> it is even needed, as h2645_parse.c already contains debug log messages
>> to display the unit type.
>>
> It's copied from hevcdec. It means we did not handle the nal diffrent than
> h2645_parser.c mes

Re: [FFmpeg-devel] [PATCH] avutil/mem: always align by at least 32 bytes

2023-12-08 Thread Andreas Rheinhardt
Timo Rothenpieler:
> On 08.12.2023 11:01, Andreas Rheinhardt wrote:
>> Timo Rothenpieler:
>>> FFmpeg has instances of DECLARE_ALIGNED(32, ...) in a lot of structs,
>>> which then end up heap-allocated.
>>> By declaring any variable in a struct, or tree of structs, to be 32 byte
>>> aligned, it allows the compiler to safely assume the entire struct
>>> itself is also 32 byte aligned.
>>>
>>> This might make the compiler emit code which straight up crashes or
>>> misbehaves in other ways, and at least in one instances is now
>>> documented to actually do (see ticket 10549 on trac).
>>> The issue there is that an unrelated variable in SingleChannelElement is
>>> declared to have an alignment of 32 bytes. So if the compiler does a
>>> copy
>>> in decode_cpe() with avx instructions, but ffmpeg is built with
>>> --disable-avx, this results in a crash, since the memory is only 16 byte
>>> aligned.
>>> Mind you, even if the compiler does not emit avx instructions, the code
>>> is still invalid and could misbehave. It just happens not to. Declaring
>>> any variable in a struct with a 32 byte alignment promises 32 byte
>>> alignment of the whole struct to the compiler.
>>>
>>> Instead of now going through all instances of variables in structs
>>> being declared as 32 byte aligned, this patch bumps the minimum
>>> alignment
>>> to 32 bytes.
>>> ---
>>>   libavutil/mem.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavutil/mem.c b/libavutil/mem.c
>>> index 36b8940a0c..26a9b9753b 100644
>>> --- a/libavutil/mem.c
>>> +++ b/libavutil/mem.c
>>> @@ -62,7 +62,7 @@ void  free(void *ptr);
>>>     #endif /* MALLOC_PREFIX */
>>>   -#define ALIGN (HAVE_AVX512 ? 64 : (HAVE_AVX ? 32 : 16))
>>> +#define ALIGN (HAVE_AVX512 ? 64 : 32)
>>>     /* NOTE: if you want to override these functions with your own
>>>    * implementations (not recommended) you have to link libav* as
>>
>> 1. There is another way in which this can be triggered: Namely if one
>> uses a build with AVX, but combines it with a lavu built without it; it
>> is also triggerable on non-x86 (having an insufficiently aligned pointer
>> is always UB even if the CPU does not have instructions that would
>> benefit from the additional alignment). You should mention this in the
>> commit message.
> 
> Is mixing the libraries really a scenario we need to care about/support?
> 

IMO, no, but Anton cares about it a lot.

> And yeah, this is only marginally related to AVX, in that it's what
> triggers a crash in this scenario.
> But as stated in the commit message, it's not a valid thing to do in any
> case, on any arch. It just happens not to crash.
> 

I know, but this patch also happens to fix this (at least to some
degree), so this should be mentioned in the commit message.

>> 2. This topic gave me headaches when creating RefStruct. I "solved" it
>> by (ab)using STRIDE_ALIGN which mimicks the alignment of av_malloc(),
>> thereby ensuring that RefStruct does not break lavc builds built with
>> the avx dsp functions enabled (but it does not guard against using a
>> lavu whose av_malloc() only provides less alignment).
>>
>> 3. There is a downside to your patch: It bumps alignment for non-x86
>> arches which wastes memory (and may make allocators slower). We could
>> fix this by modifying the 32-byte-alignment macros to only provide 16
>> byte alignment if the ARCH_ (and potentially the HAVE_) defines indicate
>> that no alignment bigger than 16 is needed.
> 
> But it's invalid on any other arch as well, just hasn't bitten us yet.

It is not invalid if we modify DECLARE_ALIGNED to never use more
alignment than 16 on non-x86 arches. Then all the other arches can
continue to use 16.

> I'm not sure if I'd want to start maintaining a growingly complex list
> of CPU extensions and make the DECLARE_ALIGNED macro lie if we think it
> doesn't need 32 byte alignment.
> We don't really know why someone wants a variable aligned after all.

I am fine with that point. Although I don't think it would be that
complicated if it is done at one point (namely in configure) and if all
the other places would just use a macro for max alignment (that would be
placed in config.h).

- 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-requ...@ffmpeg.org with subject "unsubscribe".


[FFmpeg-devel] MAINTAINERS: Remove older s3tc entry

2023-12-08 Thread Vittorio Giovara
Missed from a5b2b22d9a45c9634e6947d43945ebafe121abec.


0001-MAINTAINERS-Remove-older-s3tc-entry.patch
Description: Binary data
___
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".