diff mbox series

[FFmpeg-devel] Add support for LJ92 compressed MLV files, attempt 02

Message ID CAB+1MHrHm1gS-OC=LzuyAOTQR-k0FjJfi9WyxY4XY2ZPYAaxkQ@mail.gmail.com
State New
Headers show
Series [FFmpeg-devel] Add support for LJ92 compressed MLV files, attempt 02 | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

South East Oct. 24, 2024, 5:59 p.m. UTC
ffmpeg has existing support for MLV raw video files; libavformat/mlvdec.c.
Since this was added, MLV has been extended to support LJ92 compressed
image data.  These patches build on lossless DNG support in 7.2-dev to
enable handling LJ92 MLV via existing libavcodec/mjpegdec.c code.

Sample LJ92 MLV file:
https://0x0.st/XlFm.mlv

FATE tests pass locally.  It was not obvious from the Developer guide how
to get this working.  The claim is "Running ’make fate’ accomplishes
[running local FATE tests]", but this of course does not work until
you rsync the samples, build FATE etc, which is not mentioned:
https://www.ffmpeg.org/developer.html#Regression-tests-1

I suggest adding something like "but WILL NOT work until it is configured",
before "please see fate.html for details".  This would make it clearer
that you NEED to follow the link, rather than it existing as reference
material should you want it in the future.  Simply running "make fate"
fails, but in a way that looked like a normal test failure to me;
I assumed upstream was broken since the failure was repeatable and
occured without my changes.

There is a long comment in patch 0001 that I don't expect to survive review,
but provides context for the code change.  As an ffmpeg noob I can't
make a good judgement on what level of commenting is appropriate.

For reference, attempt 1 can be seen in the archives, here:
https://ffmpeg.org//pipermail/ffmpeg-devel/2024-October/335245.html

Comments

Tomas Härdin Oct. 29, 2024, 3:25 p.m. UTC | #1
tor 2024-10-24 klockan 18:59 +0100 skrev South East:
> ffmpeg has existing support for MLV raw video files;
> libavformat/mlvdec.c.
> Since this was added, MLV has been extended to support LJ92
> compressed
> image data.  These patches build on lossless DNG support in 7.2-dev
> to
> enable handling LJ92 MLV via existing libavcodec/mjpegdec.c code.
> 
> Sample LJ92 MLV file:
> https://0x0.st/XlFm.mlv

Looks reasonably small to fit in FATE

> -#define MLV_CLASS_FLAG_DELTA 0x40
>  #define MLV_CLASS_FLAG_LZMA  0x80
> +#define MLV_CLASS_FLAG_DELTA 0x40
> +#define MLV_CLASS_FLAG_LJ92  0x20

This hunk could be simpler if you didn't move MLV_CLASS_FLAG_DELTA

Not sure how Bayer in JPEG works so can't really comment more on patch
1.

Patch 2 looks simple enough.

/Tomas
South East Oct. 30, 2024, 4:27 a.m. UTC | #2
Thanks for taking a look!

On Tue, 29 Oct 2024 at 15:25, Tomas Härdin <git@haerdin.se> wrote:
> > -#define MLV_CLASS_FLAG_DELTA 0x40
> >  #define MLV_CLASS_FLAG_LZMA  0x80
> > +#define MLV_CLASS_FLAG_DELTA 0x40
> > +#define MLV_CLASS_FLAG_LJ92  0x20
>
> This hunk could be simpler if you didn't move MLV_CLASS_FLAG_DELTA

This preserves the order of flags in the code that creates MLV files:
https://github.com/reticulatedpines/magiclantern_simplified/blob/f7a1df28c52eb7e1725b078a5fd2c4dedb3bac85/modules/raw_video/mlv_rec/mlv.h#L32
I don't care a large amount, and can change it, but being consistent
has some value.

> Not sure how Bayer in JPEG works so can't really comment more on patch
> 1.

I don't know how Bayer in JPEG works either.  I'm not sure if you mean
the file format, or the data compression standards.  While MLV can use LJ92
compression applied to Bayer data, there are no JPEG files involved.

The existing mjpeg LJ92 support, that I'm building on, came from this work:
https://velocityra.github.io/gsoc-2019/

> Patch 2 looks simple enough.

Good, I was hoping that one would be.
South East Nov. 4, 2024, 6:14 a.m. UTC | #3
Hi all - what do I need to do to progress this?

I'm not sure, but it sounds like Tomas thought patch 1 might need
someone with experience with Bayer taking a look.

Thanks

On Wed, 30 Oct 2024 at 04:27, South East <8billion.people@gmail.com> wrote:
>
> Thanks for taking a look!
>
> On Tue, 29 Oct 2024 at 15:25, Tomas Härdin <git@haerdin.se> wrote:
> > > -#define MLV_CLASS_FLAG_DELTA 0x40
> > >  #define MLV_CLASS_FLAG_LZMA  0x80
> > > +#define MLV_CLASS_FLAG_DELTA 0x40
> > > +#define MLV_CLASS_FLAG_LJ92  0x20
> >
> > This hunk could be simpler if you didn't move MLV_CLASS_FLAG_DELTA
>
> This preserves the order of flags in the code that creates MLV files:
> https://github.com/reticulatedpines/magiclantern_simplified/blob/f7a1df28c52eb7e1725b078a5fd2c4dedb3bac85/modules/raw_video/mlv_rec/mlv.h#L32
> I don't care a large amount, and can change it, but being consistent
> has some value.
>
> > Not sure how Bayer in JPEG works so can't really comment more on patch
> > 1.
>
> I don't know how Bayer in JPEG works either.  I'm not sure if you mean
> the file format, or the data compression standards.  While MLV can use LJ92
> compression applied to Bayer data, there are no JPEG files involved.
>
> The existing mjpeg LJ92 support, that I'm building on, came from this work:
> https://velocityra.github.io/gsoc-2019/
>
> > Patch 2 looks simple enough.
>
> Good, I was hoping that one would be.
Michael Niedermayer Nov. 9, 2024, 12:37 a.m. UTC | #4
On Mon, Nov 04, 2024 at 06:14:07AM +0000, South East wrote:
> Hi all - what do I need to do to progress this?
> 
> I'm not sure, but it sounds like Tomas thought patch 1 might need
> someone with experience with Bayer taking a look.

iam a bit overloaded with work ATM, but bayer or interlacing combined with
jpeg gives me memories of segfaults. So maybe you can run this through some fuzzer
with some samples that trigger the code pathes
to check it a bit

thx

[...]
South East Nov. 9, 2024, 1:08 a.m. UTC | #5
On Sat, 9 Nov 2024 at 00:37, Michael Niedermayer <michael@niedermayer.cc> wrote:
>
> On Mon, Nov 04, 2024 at 06:14:07AM +0000, South East wrote:
> > Hi all - what do I need to do to progress this?
>
> iam a bit overloaded with work ATM, but bayer or interlacing combined with
> jpeg gives me memories of segfaults. So maybe you can run this through some fuzzer
> with some samples that trigger the code pathes
> to check it a bit

Thanks.  I have experience with AFL so this is practical for me.  The
likely output is
a collection of samples that will improve code coverage, focussing on MLV and
DNG files.

Does ffmpeg use AFL for testing already?  I would expect to make local code
modifications to ffmpeg in order to improve speed of fuzzing (see e.g.
__AFL_LOOP).
Would you want those changes?  It should be obvious they do something because
of improved code coverage, perhaps that is enough.

I would expect testing with ffplay (with an ASAN enabled build) would be an
 acceptable scope (there is no encoder for MLV).  Is that assumption correct?

I would guess we are only interested in new problems when the patches are
applied, i.e., if I discover old flaws, that shouldn't have any
bearing on whether
my patches are accepted.

Beyond that, what would you consider evidence of adequate testing?
diff mbox series

Patch

From db0f076e8f724deee604af7a1a85c1d130f1f87d Mon Sep 17 00:00:00 2001
From: stephen-e <33672591+reticulatedpines@users.noreply.github.com>
Date: Mon, 21 Oct 2024 16:35:49 +0100
Subject: [PATCH 2/2] avformat/mlvdec: add LJ92 support

MLV files can contain LJ92 compressed raw video data.
MJPEG codec can be used to handle these.
---
 libavformat/mlvdec.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/libavformat/mlvdec.c b/libavformat/mlvdec.c
index 1a6d38f37c..2eb21a3aab 100644
--- a/libavformat/mlvdec.c
+++ b/libavformat/mlvdec.c
@@ -44,8 +44,9 @@ 
 
 #define MLV_AUDIO_CLASS_WAV  1
 
-#define MLV_CLASS_FLAG_DELTA 0x40
 #define MLV_CLASS_FLAG_LZMA  0x80
+#define MLV_CLASS_FLAG_DELTA 0x40
+#define MLV_CLASS_FLAG_LJ92  0x20
 
 typedef struct {
     AVIOContext *pb[101];
@@ -298,9 +299,12 @@  static int read_header(AVFormatContext *avctx)
         if ((mlv->class[0] & (MLV_CLASS_FLAG_DELTA|MLV_CLASS_FLAG_LZMA)))
             avpriv_request_sample(avctx, "compression");
         vst->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
-        switch (mlv->class[0] & ~(MLV_CLASS_FLAG_DELTA|MLV_CLASS_FLAG_LZMA)) {
+        switch (mlv->class[0] & ~(MLV_CLASS_FLAG_DELTA|MLV_CLASS_FLAG_LZMA|MLV_CLASS_FLAG_LJ92)) {
         case MLV_VIDEO_CLASS_RAW:
-            vst->codecpar->codec_id = AV_CODEC_ID_RAWVIDEO;
+            if (mlv->class[0] & MLV_CLASS_FLAG_LJ92)
+                vst->codecpar->codec_id = AV_CODEC_ID_MJPEG;
+            else
+                vst->codecpar->codec_id = AV_CODEC_ID_RAWVIDEO;
             break;
         case MLV_VIDEO_CLASS_YUV:
             vst->codecpar->format   = AV_PIX_FMT_YUV420P;
-- 
2.45.2