bbc / bmx

Library and utilities to read and write broadcasting media files. Primarily supports the MXF file format
BSD 3-Clause "New" or "Revised" License
57 stars 16 forks source link

JPEG XS essence support #48

Closed nishabfhg closed 6 months ago

nishabfhg commented 8 months ago

Hi,

Here are my changes to include JPEG XS essence support into bmx. It works pretty much like JPEG 2000. Only PARSE_FRAME_START_SIZE has to be larger in RawEssenceReader. Let me know if I need to modify something.

Thank you. Best regards, Nisha

philipnbbc commented 8 months ago

Thanks! I don't have time available today to review but will try make a start tomorrow or Wednesday. Is there a sample available somewhere online I can use to test?

nishabfhg commented 8 months ago

Great! I don't know if there are any jxs samples available online. I have added a short RGB test sequence here: https://github.com/nishabfhg/bmx-jxs/tree/main/Blender_FullHD_Testsequence. In raw2bmx, you will need the jxs_rgba command line option in this case.

philipnbbc commented 8 months ago

Here is the list of warnings from the Linux build:

/home/runner/work/bmx/bmx/src/essence_parser/FilePatternEssenceSource.cpp: In member function ‘int64_t bmx::FilePatternEssenceSource::ReadFileSize(uint32_t)’:
/home/runner/work/bmx/bmx/src/essence_parser/FilePatternEssenceSource.cpp:208:14: warning: unused variable ‘res’ [-Wunused-variable]
  208 |         bool res = NextFile();
      |              ^~~
In file included from /home/runner/work/bmx/bmx/src/essence_parser/JXSEssenceParser.cpp:33:
/home/runner/work/bmx/bmx/src/essence_parser/JXSEssenceParser.cpp: In member function ‘virtual void bmx::JXSEssenceParser::ParseFrameInfo(const unsigned char*, uint32_t)’:
/home/runner/work/bmx/bmx/include/bmx/essence_parser/JXSEssenceParser.h:49:34: warning: comparison of unsigned expression in ‘< 0’ is always false [-Wtype-limits]
   49 |         #define SUCCESS(v) (((v) < 0) ? 0 : 1)
      |                              ~~~~^~~
/home/runner/work/bmx/bmx/src/essence_parser/JXSEssenceParser.cpp:144:29: note: in expansion of macro ‘SUCCESS’
  144 |         while (p < end_p && SUCCESS(result))
      |                             ^~~~~~~
/home/runner/work/bmx/bmx/include/bmx/essence_parser/JXSEssenceParser.h:50:34: warning: comparison of unsigned expression in ‘< 0’ is always false [-Wtype-limits]
   50 |         #define FAILURE(v) (((v) < 0) ? 1 : 0)
      |                              ~~~~^~~
/home/runner/work/bmx/bmx/src/essence_parser/JXSEssenceParser.cpp:148:21: note: in expansion of macro ‘FAILURE’
  148 |                 if (FAILURE(result))
      |                     ^~~~~~~
/home/runner/work/bmx/bmx/src/essence_parser/JXSEssenceParser.cpp:211:66: warning: comparison is always false due to limited range of data type [-Wtype-limits]
  211 |                                 if (PIH_.Hsl() < 1 || PIH_.Hsl() > 0xffff) // This includes the EOF check
      |                                                       ~~~~~~~~~~~^~~~~~~~
/home/runner/work/bmx/bmx/src/essence_parser/JXSEssenceParser.cpp:154:24: warning: enumeration value ‘MRK_NIL’ not handled in switch [-Wswitch]
  154 |                 switch (NextMarker.m_Type)
      |                        ^
/home/runner/work/bmx/bmx/src/essence_parser/JXSEssenceParser.cpp:154:24: warning: enumeration value ‘MRK_WGT’ not handled in switch [-Wswitch]
/home/runner/work/bmx/bmx/src/essence_parser/JXSEssenceParser.cpp:154:24: warning: enumeration value ‘MRK_COM’ not handled in switch [-Wswitch]
/home/runner/work/bmx/bmx/src/essence_parser/JXSEssenceParser.cpp:154:24: warning: enumeration value ‘MRK_NLT’ not handled in switch [-Wswitch]
/home/runner/work/bmx/bmx/src/essence_parser/JXSEssenceParser.cpp:154:24: warning: enumeration value ‘MRK_CWD’ not handled in switch [-Wswitch]
/home/runner/work/bmx/bmx/src/essence_parser/JXSEssenceParser.cpp:154:24: warning: enumeration value ‘MRK_CTS’ not handled in switch [-Wswitch]
/home/runner/work/bmx/bmx/src/essence_parser/JXSEssenceParser.cpp:154:24: warning: enumeration value ‘MRK_CRG’ not handled in switch [-Wswitch]
/home/runner/work/bmx/bmx/src/essence_parser/JXSEssenceParser.cpp:154:24: warning: enumeration value ‘MRK_CAP’ not handled in switch [-Wswitch]
/home/runner/work/bmx/bmx/apps/bmxtranswrap/bmxtranswrap.cpp: In function ‘int main(int, const char**)’:
/home/runner/work/bmx/bmx/apps/bmxtranswrap/bmxtranswrap.cpp:3967:20: warning: enumeration value ‘JPEGXS_CDCI’ not handled in switch [-Wswitch]
 3967 |             switch (output_essence_type)
      |                    ^
/home/runner/work/bmx/bmx/apps/bmxtranswrap/bmxtranswrap.cpp:3967:20: warning: enumeration value ‘JPEGXS_RGBA’ not handled in switch [-Wswitch]
philipnbbc commented 7 months ago

As promised, here is the process for creating regression tests for JXS.

More info on testing can be found here. This process assumes you only have an RGBA example. If you also have a CDCI example file then ignore the steps below that delete that particular part of the tests; you will have 4 test data files instead of 2.

nishabfhg commented 7 months ago

Hi Philip, thanks for the review! I will try to make all the modifications next week and get back to you.

nishabfhg commented 7 months ago

Hi Philip, I have modified the code according to your review points. I also added a regression test for jpegxs cdci. Let me know if there's anything else. (I will only be able to get back to this after the New Year holidays)

philipnbbc commented 7 months ago

I've had a quick look through and looks like you covered everything, thanks for the updates! I'll probably take a proper look sometime next week as I'm away from work.

nishabfhg commented 6 months ago

Hi Philip. I made the fixes. I hope I didn't miss anything. I synced my version with your latest commits, so the merge conflicts should not appear anymore.

philipnbbc commented 6 months ago

All done now except for some unexpected changes to some non-jxs test checksums - see below the diff showing what needs to be changed. Do you know what caused this? Just asking as it might be an issue that needs fixing.

If you run cmake --build . -t bmx_test_data and cmake --build . -t libMXF_test_data it should regenerate the checksums as bbelow. cmake --build . -t test should then pass.

Diff showing what needs to be changed back:

diff --git a/deps/libMXF/test/test_indextable.md5 b/deps/libMXF/test/test_indextable.md5
index e781305e..6dc5d47a 100644
--- a/deps/libMXF/test/test_indextable.md5
+++ b/deps/libMXF/test/test_indextable.md5
@@ -1 +1 @@
-d41d8cd98f00b204e9800998ecf8427e
\ No newline at end of file
+5ff6163c7004300edb5e0c6670e547d3
\ No newline at end of file
diff --git a/test/as02/iecdv25.md5s b/test/as02/iecdv25.md5s
index ec6da267..8e831eb3 100644
--- a/test/as02/iecdv25.md5s
+++ b/test/as02/iecdv25.md5s
@@ -1 +1 @@
-abce8e563ec3408a0dbf10ae34dfe30c;ca114c9f2707afde489d6e6066f8833c;e4a2bdff4418802e3d127654ee906dc6;99538bb806a99596b95236ac8a965b52
\ No newline at end of file
+227a7e8a6b34571cfb3a9b70642c0183;4e4605266cb9d916830ea1eef722321d;d8b021e1b8bc3bd3796676b0b5e8f594;16ff89dda611b560eda0739fd90d0214
\ No newline at end of file
diff --git a/test/avid_mxf/mpeg2lg_422p_hl_1080i.md5s b/test/avid_mxf/mpeg2lg_422p_hl_1080i.md5s
index 606895d7..8e1e6d3e 100644
--- a/test/avid_mxf/mpeg2lg_422p_hl_1080i.md5s
+++ b/test/avid_mxf/mpeg2lg_422p_hl_1080i.md5s
@@ -1 +1 @@
-f95eae8d685f5c222c7e3e9fb84b9f9a;419a18c68d4b87dfb10408653a6bc58e;805d6fd84c1ed39585053c49016f52f7
\ No newline at end of file
+f189845c35fbe0a69d36b181d85937f9;0ece8877ab256657df705de6a01a8a48;f323a74f1505abc8bf201eeef07e0089
\ No newline at end of file
diff --git a/test/avid_mxf/mpeg2lg_mp_hl_1920_1080i.md5s b/test/avid_mxf/mpeg2lg_mp_hl_1920_1080i.md5s
index 2b2e3f30..793a0a50 100644
--- a/test/avid_mxf/mpeg2lg_mp_hl_1920_1080i.md5s
+++ b/test/avid_mxf/mpeg2lg_mp_hl_1920_1080i.md5s
@@ -1 +1 @@
-93d9e5755cdcdd13000b71bcad7d55d1;a31f0491c9255156d07ec78ac265311c;f3259501cd9d48ce94f5f1aa0006d697
\ No newline at end of file
+fbd8e5365fbd58c4a4908cd2e78319f7;0ece8877ab256657df705de6a01a8a48;f323a74f1505abc8bf201eeef07e0089
\ No newline at end of file
diff --git a/test/d10_mxf/d10_40_2997.md5 b/test/d10_mxf/d10_40_2997.md5
index 20d6158e..8563ad8d 100644
--- a/test/d10_mxf/d10_40_2997.md5
+++ b/test/d10_mxf/d10_40_2997.md5
@@ -1 +1 @@
-8a256f169a5cbaa6145f95a9d008df0a
\ No newline at end of file
+6e62f6adec11065622203c7e75cc2b38
\ No newline at end of file
nishabfhg commented 6 months ago

The md5s might have changed when I rebuilt the project using the visual studio solution. I am not sure why this happened.

nishabfhg commented 6 months ago

That's great! Thanks for the review.