[FFmpeg-devel] [PATCH 1/3] avformat/matroska: Support JPEG2000 for demuxing

[FFmpeg-devel] [PATCH 1/3] avformat/matroska: Support JPEG2000 for demuxing

[Andreas Rheinhardt]

[-- Attachment #1: Type: text/plain, Size: 29 bytes --]

Patches attached.

- Andreas

[-- Attachment #2: 0001-avformat-matroska-Support-JPEG2000-for-demuxing.patch --]
[-- Type: text/x-patch, Size: 2055 bytes --]

From b58c71c2e90380d8a3e7a58fa20a6c16c2458317 Mon Sep 17 00:00:00 2001
From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Date: Sun, 25 May 2025 03:31:17 +0200
Subject: [PATCH 1/3] avformat/matroska: Support JPEG2000 for demuxing

Legal since commit 1cd0a9be4b2d1e7c60184ec68404e00e46e3123e
(Jan 4) in the Cellar Matroska specification git repo.
We still hold out on muxing it due to compatibility with
old demuxers.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/matroska.c    | 1 +
 libavformat/matroskaenc.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/libavformat/matroska.c b/libavformat/matroska.c
index bbad9a7f54..60584e2687 100644
--- a/libavformat/matroska.c
+++ b/libavformat/matroska.c
@@ -82,6 +82,7 @@ const CodecTags ff_mkv_codec_tags[]={
     {"V_AVS3"           , AV_CODEC_ID_AVS3},
     {"V_DIRAC"          , AV_CODEC_ID_DIRAC},
     {"V_FFV1"           , AV_CODEC_ID_FFV1},
+    {"V_JPEG2000"       , AV_CODEC_ID_JPEG2000},
     {"V_MJPEG"          , AV_CODEC_ID_MJPEG},
     {"V_MPEG1"          , AV_CODEC_ID_MPEG1VIDEO},
     {"V_MPEG2"          , AV_CODEC_ID_MPEG2VIDEO},
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 6d0d791f18..9d13f74907 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -1960,8 +1960,8 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
 
         // look for a codec ID string specific to mkv to use,
         // if none are found, use AVI codes
-        if (par->codec_id == AV_CODEC_ID_FFV1) {
-            /* FFV1 is actually supported natively in Matroska,
+        if (par->codec_id == AV_CODEC_ID_FFV1 || par->codec_id == AV_CODEC_ID_JPEG2000) {
+            /* FFV1 and JPEG2000 are actually supported natively in Matroska,
              * yet we use the VfW way to mux it for compatibility
              * with old demuxers. (FIXME: Are they really important?) */
         } else if (par->codec_id != AV_CODEC_ID_RAWVIDEO || par->codec_tag) {
-- 
2.45.2


[-- Attachment #3: 0002-avformat-matroskadec-Fix-VfW-extradata-size.patch --]
[-- Type: text/x-patch, Size: 2113 bytes --]

From 5d8954294fee0d1086047ae2587856c3afb7ff7c Mon Sep 17 00:00:00 2001
From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Date: Sun, 25 May 2025 05:12:05 +0200
Subject: [PATCH 2/3] avformat/matroskadec: Fix VfW extradata size

The structure is padded to an even length with an internal
size field to indicate the real size.
The matroska-matroska-display-metadata test (writing FFV1
in VFW mode) was affected by this.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/matroskadec.c                          | 5 +++++
 tests/ref/fate/matroska-mastering-display-metadata | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 2750652c51..da5166319e 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -2877,6 +2877,11 @@ static int mkv_parse_video_codec(MatroskaTrack *track, AVCodecParameters *par,
 {
     if (!strcmp(track->codec_id, "V_MS/VFW/FOURCC") &&
         track->codec_priv.size >= 40) {
+        uint32_t size = AV_RL32A(track->codec_priv.data);
+        // VFW extradata is padded to an even length, yet
+        // the size field contains the real length.
+        if (size & 1 && size == track->codec_priv.size - 1)
+            --track->codec_priv.size;
         track->ms_compat    = 1;
         par->bits_per_coded_sample = AV_RL16(track->codec_priv.data + 14);
         par->codec_tag      = AV_RL32(track->codec_priv.data + 16);
diff --git a/tests/ref/fate/matroska-mastering-display-metadata b/tests/ref/fate/matroska-mastering-display-metadata
index 6a2ff15b1b..5984f54131 100644
--- a/tests/ref/fate/matroska-mastering-display-metadata
+++ b/tests/ref/fate/matroska-mastering-display-metadata
@@ -1,7 +1,7 @@
 c1e5e2ecf433cf05af8556debc7d4d0b *tests/data/fate/matroska-mastering-display-metadata.matroska
 1669773 tests/data/fate/matroska-mastering-display-metadata.matroska
 #extradata 0:        4, 0x040901a3
-#extradata 3:      202, 0xfce96279
+#extradata 3:      201, 0x9a706279
 #tb 0: 1/1000
 #media_type 0: video
 #codec_id 0: prores
-- 
2.45.2


[-- Attachment #4: 0003-avformat-matroskaenc-Use-native-id-instead-of-VfW-fo.patch --]
[-- Type: text/x-patch, Size: 3257 bytes --]

From 5ac5da0a6e97304b678c08d7047d3b19f12817ac Mon Sep 17 00:00:00 2001
From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
Date: Sun, 25 May 2025 04:26:20 +0200
Subject: [PATCH 3/3] avformat/matroskaenc: Use native id instead of VfW for
 FFV1

Up until now, our muxer wrote FFV1 in video-for-windows
compatibility mode out of concern for old demuxers that
only support that (whereas the demuxer accepts V_FFV1).
This commit switches to using native mode, because
a) V_FFV1 is around long enough so that old demuxers
should not be an issue (support in FFmpeg has been added
in commit 9ae762da7e256aa4d3b645c614fcd1959e1cbb8d
in March 2017/FFmpeg 3.3),
b) using native mode uses fewer bytes for the CodecPrivate,
c) the VfW extradata is zero-padded to an even length
if necessary, but our demuxer forgot to undo the padding
until very recently, so that there are many versions of
our demuxer around that are buggy wrt VFW, but not V_FFV1.
This affects the FFV1 extradata checksums, specifically
the (experimental) version 4 files with error check version 2*
as created by
ffmpeg -i ../fate-suite/mpeg2/sony-ct3.bs -c:v ffv1 \
-slices 16 -frames 1 -level 4 -strict experimental ffv1.mkv
VFW files like the above created by this muxer before this patch
would not work with an old demuxer.

*: without error check version 2, the CRC for the whole extradata
is zero, which is not changed by appending a zero byte.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/matroskaenc.c                          | 4 ++--
 tests/ref/fate/matroska-mastering-display-metadata | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 9d13f74907..408890fa89 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -1960,8 +1960,8 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
 
         // look for a codec ID string specific to mkv to use,
         // if none are found, use AVI codes
-        if (par->codec_id == AV_CODEC_ID_FFV1 || par->codec_id == AV_CODEC_ID_JPEG2000) {
-            /* FFV1 and JPEG2000 are actually supported natively in Matroska,
+        if (par->codec_id == AV_CODEC_ID_JPEG2000) {
+            /* JPEG2000 is actually supported natively in Matroska,
              * yet we use the VfW way to mux it for compatibility
              * with old demuxers. (FIXME: Are they really important?) */
         } else if (par->codec_id != AV_CODEC_ID_RAWVIDEO || par->codec_tag) {
diff --git a/tests/ref/fate/matroska-mastering-display-metadata b/tests/ref/fate/matroska-mastering-display-metadata
index 5984f54131..6f10dc57a6 100644
--- a/tests/ref/fate/matroska-mastering-display-metadata
+++ b/tests/ref/fate/matroska-mastering-display-metadata
@@ -1,5 +1,5 @@
-c1e5e2ecf433cf05af8556debc7d4d0b *tests/data/fate/matroska-mastering-display-metadata.matroska
-1669773 tests/data/fate/matroska-mastering-display-metadata.matroska
+bdca53906b34c57192416a0f737b885e *tests/data/fate/matroska-mastering-display-metadata.matroska
+1669723 tests/data/fate/matroska-mastering-display-metadata.matroska
 #extradata 0:        4, 0x040901a3
 #extradata 3:      201, 0x9a706279
 #tb 0: 1/1000
-- 
2.45.2


[-- Attachment #5: Type: text/plain, Size: 251 bytes --]

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

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

Re: [FFmpeg-devel] [PATCH 1/3] avformat/matroska: Support JPEG2000 for demuxing

[compn]

On Sun, 25 May 2025 05:50:54 +0200, Andreas Rheinhardt wrote:

> Patches attached.
> 
> - Andreas

looks ok but i wonder if anyone is using the vfw ffv1 mkv in the
archiving universe on purpose. should we include a way to -vfw-codec-id
manual for those systems? maybe is unimportant but it would be good to
ask the ffv1 specification crowd about this change to mkv muxing. and
maybe resend patch separately from jpeg2000 decoding subject. hiding an
ffv1 change under jpeg2000 subj is weird.

said another another way, this change would change the checksums of
files if people tried to recreate their mkv. which is something that
archivists do sometimes.

e.g. old ffv1 mkv vfw codec_id
vs
new ffv1 mkv codec_id

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

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

Re: [FFmpeg-devel] [PATCH 1/3] avformat/matroska: Support JPEG2000 for demuxing

[Andreas Rheinhardt]

compn:
> On Sun, 25 May 2025 05:50:54 +0200, Andreas Rheinhardt wrote:
> 
>> Patches attached.
>>
>> - Andreas
> 
> looks ok but i wonder if anyone is using the vfw ffv1 mkv in the
> archiving universe on purpose. should we include a way to -vfw-codec-id
> manual for those systems? maybe is unimportant but it would be good to
> ask the ffv1 specification crowd about this change to mkv muxing. and
> maybe resend patch separately from jpeg2000 decoding subject. hiding an
> ffv1 change under jpeg2000 subj is weird.

Why should the FFV1 specification crowd object to us writing FFV1 files
in the way they have been intended to be written since February 2017
(since
https://github.com/ietf-wg-cellar/matroska-specification/commit/51a600106bfa66f58a7f2c8e05fab091ab059ebd)?
Anyway, I cc'ed Dave Rice, the author of the aforementioned commit.

> 
> said another another way, this change would change the checksums of
> files if people tried to recreate their mkv. which is something that
> archivists do sometimes.

That's generally what happens when you update your FFmpeg, not only for
the Matroska muxer, but for any component that is under active
development. We don't give guarantees that the output stays the same
when using different git snapshots.
Specifically for Matroska, there have been multiple commits changing the
output of basically every Matroska file created by our muxer (often by
using fewer bytes to write length fields); see e.g. git log
tests/ref/fate/matroska-flac-extradata-update
(Btw: The second patch in this set shows that simply remuxing is
dangerous for FFV1 in VFW-mode.)

- Andreas

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

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

Re: [FFmpeg-devel] [PATCH 1/3] avformat/matroska: Support JPEG2000 for demuxing

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Patches attached.
> 
> - Andreas
> 
Will apply and backport the first two patches of this patchset unless
there are objections.

- Andreas

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

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

Re: [FFmpeg-devel] [PATCH 1/3] avformat/matroska: Support JPEG2000 for demuxing

[Dave Rice]

Hi,

> On May 25, 2025, at 7:50 PM, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> 
> compn:
>> On Sun, 25 May 2025 05:50:54 +0200, Andreas Rheinhardt wrote:
>> 
>>> Patches attached.
>>> 
>>> - Andreas
>> 
>> looks ok but i wonder if anyone is using the vfw ffv1 mkv in the
>> archiving universe on purpose. should we include a way to -vfw-codec-id
>> manual for those systems? maybe is unimportant but it would be good to
>> ask the ffv1 specification crowd about this change to mkv muxing. and
>> maybe resend patch separately from jpeg2000 decoding subject. hiding an
>> ffv1 change under jpeg2000 subj is weird.
> 
> Why should the FFV1 specification crowd object to us writing FFV1 files
> in the way they have been intended to be written since February 2017
> (since
> https://github.com/ietf-wg-cellar/matroska-specification/commit/51a600106bfa66f58a7f2c8e05fab091ab059ebd)?
> Anyway, I cc'ed Dave Rice, the author of the aforementioned commit.

Thanks for the cc. I’m here linking to related conversations on this. The V_FFV1 mapping was drafted in 2017 and besides this referenced commit, there’s a discussion about it at https://mailarchive.ietf.org/arch/msg/cellar/Kir-8JdOl6DTZFPrxy4XM-iRP7Q/ with feedback from others in the IETF Cellar working group.

At that time I had also posted a request to read the V_FFV1 codec id in https://trac.ffmpeg.org/ticket/6206 which Carl Eugen Hoyos resolved and then also copied the request into VLC.

The idea was to define a V_FFV1 codec mapping, promote support for reading that in the primary Matroska tools, then wait a certain length of time, then promote writing FFV1 / matroska with that codec mapping. I forgot to set a timer for that time between updating readers and updating writers, but at 8 years I suggest it’s time for this change.

>> said another another way, this change would change the checksums of
>> files if people tried to recreate their mkv. which is something that
>> archivists do sometimes.
> 
> That's generally what happens when you update your FFmpeg, not only for
> the Matroska muxer, but for any component that is under active
> development. We don't give guarantees that the output stays the same
> when using different git snapshots.

I agree with Andreas' sentiment. I think an expectation that the different versions of the same tool produce identical results under default options would be unhelpful and we’d lose some of the benefits of the ongoing improvement to that tool and the underlying standards. For instance, several years back the FFmpeg Matroska muxer enabled embedded top-level Element CRC’s by default. Now whenever a relatively modern Matroska file from FFmpeg has a bit flip somewhere, we can determine what element is impacted.

+1 to switch from defaulting to V_FFV1 over VFW mode.

> Specifically for Matroska, there have been multiple commits changing the
> output of basically every Matroska file created by our muxer (often by
> using fewer bytes to write length fields); see e.g. git log
> tests/ref/fate/matroska-flac-extradata-update
> (Btw: The second patch in this set shows that simply remuxing is
> dangerous for FFV1 in VFW-mode.)

[…]

Thanks Andreas!
Kind Regards,
Dave Rice

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

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

Re: [FFmpeg-devel] [PATCH 1/3] avformat/matroska: Support JPEG2000 for demuxing

[compn]

On Mon, 26 May 2025 01:50:42 +0200, Andreas Rheinhardt wrote:

> Why should the FFV1 specification crowd object to us writing FFV1 files
> in the way they have been intended to be written since February 2017
> (since
> https://github.com/ietf-wg-cellar/matroska-specification/commit/51a600106bfa66f58a7f2c8e05fab091ab059ebd)?
> Anyway, I cc'ed Dave Rice, the author of the aforementioned commit.

thanks for cc'ing dave, waiting for dave's response, and thanks to dave
for the quick reply, review and OK.

i know ffv1 users are all about bitexact-ness, and i didnt know when
Dave's timers were ready for this change either.

thanks everyone. and i hope that you dont think i'm trying to slow down
or block any changes to default ffmpeg improvements. i just wanted to be
inclusive of ffv1 users opinions, and now we're set! :)

maybe i'd change commit message from
avformat/matroskaenc: Use native id instead of VfW for FFV1
to 
avformat/matroskaenc: Use native id V_FFV1 instead of V_MS/VFW/FOURCC/FFV1

if you think its appropriate anyway.

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

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

Re: [FFmpeg-devel] [PATCH 1/3] avformat/matroska: Support JPEG2000 for demuxing

[Andreas Rheinhardt]

compn:
> maybe i'd change commit message from
> avformat/matroskaenc: Use native id instead of VfW for FFV1
> to 
> avformat/matroskaenc: Use native id V_FFV1 instead of V_MS/VFW/FOURCC/FFV1

This would be too long; and inexact: the codec id stored in VFW is
"V_MS/VFW/FOURCC" (the actual codec is then given by reading a fourcc in
the CodecPrivate). Maybe "avformat/matroskaenc: Use native id V_FFV1
instead of V_MS/VFW/FOURCC". But even that is a bit long.

- Andreas

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

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