diff mbox series

[FFmpeg-devel,v4,4/4] fate/jpegxl_anim: add demuxer fate test for jpegxl_anim

Message ID 20230626154922.66550-5-leo.izen@gmail.com
State New
Headers show
Series JPEG XL Parser | expand

Checks

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

Commit Message

Leo Izen June 26, 2023, 3:49 p.m. UTC 
Adds a fate test for the jpegxl_anim demuxer, that should allow testing
for true positives and false positives for animated jpegxl files. Note
that two of the test cases are not animated, in order to help sort out
false positives.

Signed-off-by: <leo.izen@gmail.com>
---
 tests/Makefile                         |  1 +
 tests/fate/jxl.mak                     | 16 ++++++++++++++++
 tests/ref/fate/jxl-anim-demux-belgium  |  6 ++++++
 tests/ref/fate/jxl-anim-demux-icos4d   |  6 ++++++
 tests/ref/fate/jxl-anim-demux-lenna256 |  7 +++++++
 tests/ref/fate/jxl-anim-demux-newton   |  6 ++++++
 6 files changed, 42 insertions(+)
 create mode 100644 tests/fate/jxl.mak
 create mode 100644 tests/ref/fate/jxl-anim-demux-belgium
 create mode 100644 tests/ref/fate/jxl-anim-demux-icos4d
 create mode 100644 tests/ref/fate/jxl-anim-demux-lenna256
 create mode 100644 tests/ref/fate/jxl-anim-demux-newton

Comments

James Almer June 27, 2023, 11:25 p.m. UTC | #1 
On 6/26/2023 12:49 PM, Leo Izen wrote:
> Adds a fate test for the jpegxl_anim demuxer, that should allow testing
> for true positives and false positives for animated jpegxl files. Note
> that two of the test cases are not animated, in order to help sort out
> false positives.
> 
> Signed-off-by: <leo.izen@gmail.com>
> ---
>   tests/Makefile                         |  1 +
>   tests/fate/jxl.mak                     | 16 ++++++++++++++++
>   tests/ref/fate/jxl-anim-demux-belgium  |  6 ++++++
>   tests/ref/fate/jxl-anim-demux-icos4d   |  6 ++++++
>   tests/ref/fate/jxl-anim-demux-lenna256 |  7 +++++++
>   tests/ref/fate/jxl-anim-demux-newton   |  6 ++++++
>   6 files changed, 42 insertions(+)
>   create mode 100644 tests/fate/jxl.mak
>   create mode 100644 tests/ref/fate/jxl-anim-demux-belgium
>   create mode 100644 tests/ref/fate/jxl-anim-demux-icos4d
>   create mode 100644 tests/ref/fate/jxl-anim-demux-lenna256
>   create mode 100644 tests/ref/fate/jxl-anim-demux-newton
> 
> diff --git a/tests/Makefile b/tests/Makefile
> index e09f30a0fc..7b80762e81 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -201,6 +201,7 @@ include $(SRC_PATH)/tests/fate/image.mak
>   include $(SRC_PATH)/tests/fate/imf.mak
>   include $(SRC_PATH)/tests/fate/indeo.mak
>   include $(SRC_PATH)/tests/fate/jpeg2000.mak
> +include $(SRC_PATH)/tests/fate/jxl.mak
>   include $(SRC_PATH)/tests/fate/libavcodec.mak
>   include $(SRC_PATH)/tests/fate/libavdevice.mak
>   include $(SRC_PATH)/tests/fate/libavformat.mak
> diff --git a/tests/fate/jxl.mak b/tests/fate/jxl.mak
> new file mode 100644
> index 0000000000..057d3be0e1
> --- /dev/null
> +++ b/tests/fate/jxl.mak
> @@ -0,0 +1,16 @@
> +# These two are animated JXL files
> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-newton
> +fate-jxl-anim-demux-newton: CMD = framecrc -i $(TARGET_SAMPLES)/jxl/newton.jxl -c copy
> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-icos4d
> +fate-jxl-anim-demux-icos4d: CMD = framecrc -i $(TARGET_SAMPLES)/jxl/icos4d.jxl -c copy
> +
> +# These two are not animated JXL. They are here to check false positives.
> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-belgium
> +fate-jxl-anim-demux-belgium: CMD = framecrc -i $(TARGET_SAMPLES)/jxl/belgium.jxl -c copy
> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-lenna256
> +fate-jxl-anim-demux-lenna256: CMD = framecrc -i $(TARGET_SAMPLES)/jxl/lenna-256.jxl -c copy
> +
> +FATE_JPEGXL_ANIM_DEMUX += $(FATE_JPEGXL_ANIM_DEMUX-yes)
> +
> +FATE_SAMPLES_FFMPEG-$(call FRAMECRC, JPEGXL_ANIM) += $(FATE_JPEGXL_ANIM_DEMUX)
> +fate-jxl-anim-demux: $(FATE_JPEGXL_ANIM_DEMUX)
> diff --git a/tests/ref/fate/jxl-anim-demux-belgium b/tests/ref/fate/jxl-anim-demux-belgium
> new file mode 100644
> index 0000000000..b2fe5035ac
> --- /dev/null
> +++ b/tests/ref/fate/jxl-anim-demux-belgium
> @@ -0,0 +1,6 @@
> +#tb 0: 1/25
> +#media_type 0: video
> +#codec_id 0: jpegxl
> +#dimensions 0: 768x512
> +#sar 0: 0/1
> +0,          0,          0,        1,       32, 0xa2930a20
> diff --git a/tests/ref/fate/jxl-anim-demux-icos4d b/tests/ref/fate/jxl-anim-demux-icos4d
> new file mode 100644
> index 0000000000..eff6ff1f1b
> --- /dev/null
> +++ b/tests/ref/fate/jxl-anim-demux-icos4d
> @@ -0,0 +1,6 @@
> +#tb 0: 1/1000
> +#media_type 0: video
> +#codec_id 0: jpegxl
> +#dimensions 0: 48x48
> +#sar 0: 0/1
> +0,          0,          0,        0,    67898, 0x53b6516b
> diff --git a/tests/ref/fate/jxl-anim-demux-lenna256 b/tests/ref/fate/jxl-anim-demux-lenna256
> new file mode 100644
> index 0000000000..0bd286a451
> --- /dev/null
> +++ b/tests/ref/fate/jxl-anim-demux-lenna256
> @@ -0,0 +1,7 @@
> +#tb 0: 1/25
> +#media_type 0: video
> +#codec_id 0: jpegxl
> +#dimensions 0: 256x256
> +#sar 0: 0/1
> +0,          0,          0,        1,     4096, 0x2409e9e3
> +0,          1,          1,        1,     3992, 0x966dbfcb

What this tells me is that the parser needs to do bitstream assembly 
after all. Image2 should not propagate a single image split in two 
packets like this, at the arbitrary limit of 4kb.

Since this format seems to have actual delimiters 
(FF_JPEGXL_CODESTREAM_SIGNATURE_LE and FF_JPEGXL_CONTAINER_SIGNATURE_LE) 
and even buffer bytes with ff_jpegxl_collect_codestream_header(), you 
should then do the assembly in the parser, much like it's done for bmp, 
jpg and many other image formats.
The anim demuxer can remain as PARSE_HEADERS so it doesn't run the frame 
assembly code.

> diff --git a/tests/ref/fate/jxl-anim-demux-newton b/tests/ref/fate/jxl-anim-demux-newton
> new file mode 100644
> index 0000000000..6fcb85c41e
> --- /dev/null
> +++ b/tests/ref/fate/jxl-anim-demux-newton
> @@ -0,0 +1,6 @@
> +#tb 0: 1/1000
> +#media_type 0: video
> +#codec_id 0: jpegxl
> +#dimensions 0: 128x96
> +#sar 0: 0/1
> +0,          0,          0,        0,    43376, 0xb2296182
Leo Izen June 27, 2023, 11:32 p.m. UTC | #2 
On 6/27/23 19:25, James Almer wrote:
> On 6/26/2023 12:49 PM, Leo Izen wrote:
>> Adds a fate test for the jpegxl_anim demuxer, that should allow testing
>> for true positives and false positives for animated jpegxl files. Note
>> that two of the test cases are not animated, in order to help sort out
>> false positives.
>>
>> Signed-off-by: <leo.izen@gmail.com>
>> ---
>>   tests/Makefile                         |  1 +
>>   tests/fate/jxl.mak                     | 16 ++++++++++++++++
>>   tests/ref/fate/jxl-anim-demux-belgium  |  6 ++++++
>>   tests/ref/fate/jxl-anim-demux-icos4d   |  6 ++++++
>>   tests/ref/fate/jxl-anim-demux-lenna256 |  7 +++++++
>>   tests/ref/fate/jxl-anim-demux-newton   |  6 ++++++
>>   6 files changed, 42 insertions(+)
>>   create mode 100644 tests/fate/jxl.mak
>>   create mode 100644 tests/ref/fate/jxl-anim-demux-belgium
>>   create mode 100644 tests/ref/fate/jxl-anim-demux-icos4d
>>   create mode 100644 tests/ref/fate/jxl-anim-demux-lenna256
>>   create mode 100644 tests/ref/fate/jxl-anim-demux-newton
>>
>> diff --git a/tests/Makefile b/tests/Makefile
>> index e09f30a0fc..7b80762e81 100644
>> --- a/tests/Makefile
>> +++ b/tests/Makefile
>> @@ -201,6 +201,7 @@ include $(SRC_PATH)/tests/fate/image.mak
>>   include $(SRC_PATH)/tests/fate/imf.mak
>>   include $(SRC_PATH)/tests/fate/indeo.mak
>>   include $(SRC_PATH)/tests/fate/jpeg2000.mak
>> +include $(SRC_PATH)/tests/fate/jxl.mak
>>   include $(SRC_PATH)/tests/fate/libavcodec.mak
>>   include $(SRC_PATH)/tests/fate/libavdevice.mak
>>   include $(SRC_PATH)/tests/fate/libavformat.mak
>> diff --git a/tests/fate/jxl.mak b/tests/fate/jxl.mak
>> new file mode 100644
>> index 0000000000..057d3be0e1
>> --- /dev/null
>> +++ b/tests/fate/jxl.mak
>> @@ -0,0 +1,16 @@
>> +# These two are animated JXL files
>> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-newton
>> +fate-jxl-anim-demux-newton: CMD = framecrc -i 
>> $(TARGET_SAMPLES)/jxl/newton.jxl -c copy
>> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-icos4d
>> +fate-jxl-anim-demux-icos4d: CMD = framecrc -i 
>> $(TARGET_SAMPLES)/jxl/icos4d.jxl -c copy
>> +
>> +# These two are not animated JXL. They are here to check false 
>> positives.
>> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-belgium
>> +fate-jxl-anim-demux-belgium: CMD = framecrc -i 
>> $(TARGET_SAMPLES)/jxl/belgium.jxl -c copy
>> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-lenna256
>> +fate-jxl-anim-demux-lenna256: CMD = framecrc -i 
>> $(TARGET_SAMPLES)/jxl/lenna-256.jxl -c copy
>> +
>> +FATE_JPEGXL_ANIM_DEMUX += $(FATE_JPEGXL_ANIM_DEMUX-yes)
>> +
>> +FATE_SAMPLES_FFMPEG-$(call FRAMECRC, JPEGXL_ANIM) += 
>> $(FATE_JPEGXL_ANIM_DEMUX)
>> +fate-jxl-anim-demux: $(FATE_JPEGXL_ANIM_DEMUX)
>> diff --git a/tests/ref/fate/jxl-anim-demux-belgium 
>> b/tests/ref/fate/jxl-anim-demux-belgium
>> new file mode 100644
>> index 0000000000..b2fe5035ac
>> --- /dev/null
>> +++ b/tests/ref/fate/jxl-anim-demux-belgium
>> @@ -0,0 +1,6 @@
>> +#tb 0: 1/25
>> +#media_type 0: video
>> +#codec_id 0: jpegxl
>> +#dimensions 0: 768x512
>> +#sar 0: 0/1
>> +0,          0,          0,        1,       32, 0xa2930a20
>> diff --git a/tests/ref/fate/jxl-anim-demux-icos4d 
>> b/tests/ref/fate/jxl-anim-demux-icos4d
>> new file mode 100644
>> index 0000000000..eff6ff1f1b
>> --- /dev/null
>> +++ b/tests/ref/fate/jxl-anim-demux-icos4d
>> @@ -0,0 +1,6 @@
>> +#tb 0: 1/1000
>> +#media_type 0: video
>> +#codec_id 0: jpegxl
>> +#dimensions 0: 48x48
>> +#sar 0: 0/1
>> +0,          0,          0,        0,    67898, 0x53b6516b
>> diff --git a/tests/ref/fate/jxl-anim-demux-lenna256 
>> b/tests/ref/fate/jxl-anim-demux-lenna256
>> new file mode 100644
>> index 0000000000..0bd286a451
>> --- /dev/null
>> +++ b/tests/ref/fate/jxl-anim-demux-lenna256
>> @@ -0,0 +1,7 @@
>> +#tb 0: 1/25
>> +#media_type 0: video
>> +#codec_id 0: jpegxl
>> +#dimensions 0: 256x256
>> +#sar 0: 0/1
>> +0,          0,          0,        1,     4096, 0x2409e9e3
>> +0,          1,          1,        1,     3992, 0x966dbfcb
> 
> What this tells me is that the parser needs to do bitstream assembly 
> after all. Image2 should not propagate a single image split in two 
> packets like this, at the arbitrary limit of 4kb.
> 
> Since this format seems to have actual delimiters 
> (FF_JPEGXL_CODESTREAM_SIGNATURE_LE and FF_JPEGXL_CONTAINER_SIGNATURE_LE) 
> and even buffer bytes with ff_jpegxl_collect_codestream_header(), you 
> should then do the assembly in the parser, much like it's done for bmp, 
> jpg and many other image formats.
> The anim demuxer can remain as PARSE_HEADERS so it doesn't run the frame 
> assembly code.
> 

The codestream signature is 0xFF 0x0A, which can and frequently will 
occur unescaped in the middle of a valid file. Detecting the end of the 
file is also a Hard Problem as it requires an entropy decoder, which I 
believe you NAK'd for being overly complex for a parser in the first 
iteration of this a few months ago.

If I choose to assemble a bitstream here with the parser, what will end 
up happening is that the entire sequence of all input frames will be one 
large AVPacket, as detecting the end of the image is nontrivial. Is this 
behavior desirable, compared to what image2dec does, which is emit 
arbitrary 4k-sized packets?

If so, I can change it to assemble a packet, but I just want to make 
sure that behavior is desirable over what I have here.

- Leo Izen
James Almer June 27, 2023, 11:46 p.m. UTC | #3 
On 6/27/2023 8:32 PM, Leo Izen wrote:
> On 6/27/23 19:25, James Almer wrote:
>> On 6/26/2023 12:49 PM, Leo Izen wrote:
>>> Adds a fate test for the jpegxl_anim demuxer, that should allow testing
>>> for true positives and false positives for animated jpegxl files. Note
>>> that two of the test cases are not animated, in order to help sort out
>>> false positives.
>>>
>>> Signed-off-by: <leo.izen@gmail.com>
>>> ---
>>>   tests/Makefile                         |  1 +
>>>   tests/fate/jxl.mak                     | 16 ++++++++++++++++
>>>   tests/ref/fate/jxl-anim-demux-belgium  |  6 ++++++
>>>   tests/ref/fate/jxl-anim-demux-icos4d   |  6 ++++++
>>>   tests/ref/fate/jxl-anim-demux-lenna256 |  7 +++++++
>>>   tests/ref/fate/jxl-anim-demux-newton   |  6 ++++++
>>>   6 files changed, 42 insertions(+)
>>>   create mode 100644 tests/fate/jxl.mak
>>>   create mode 100644 tests/ref/fate/jxl-anim-demux-belgium
>>>   create mode 100644 tests/ref/fate/jxl-anim-demux-icos4d
>>>   create mode 100644 tests/ref/fate/jxl-anim-demux-lenna256
>>>   create mode 100644 tests/ref/fate/jxl-anim-demux-newton
>>>
>>> diff --git a/tests/Makefile b/tests/Makefile
>>> index e09f30a0fc..7b80762e81 100644
>>> --- a/tests/Makefile
>>> +++ b/tests/Makefile
>>> @@ -201,6 +201,7 @@ include $(SRC_PATH)/tests/fate/image.mak
>>>   include $(SRC_PATH)/tests/fate/imf.mak
>>>   include $(SRC_PATH)/tests/fate/indeo.mak
>>>   include $(SRC_PATH)/tests/fate/jpeg2000.mak
>>> +include $(SRC_PATH)/tests/fate/jxl.mak
>>>   include $(SRC_PATH)/tests/fate/libavcodec.mak
>>>   include $(SRC_PATH)/tests/fate/libavdevice.mak
>>>   include $(SRC_PATH)/tests/fate/libavformat.mak
>>> diff --git a/tests/fate/jxl.mak b/tests/fate/jxl.mak
>>> new file mode 100644
>>> index 0000000000..057d3be0e1
>>> --- /dev/null
>>> +++ b/tests/fate/jxl.mak
>>> @@ -0,0 +1,16 @@
>>> +# These two are animated JXL files
>>> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-newton
>>> +fate-jxl-anim-demux-newton: CMD = framecrc -i 
>>> $(TARGET_SAMPLES)/jxl/newton.jxl -c copy
>>> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-icos4d
>>> +fate-jxl-anim-demux-icos4d: CMD = framecrc -i 
>>> $(TARGET_SAMPLES)/jxl/icos4d.jxl -c copy
>>> +
>>> +# These two are not animated JXL. They are here to check false 
>>> positives.
>>> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-belgium
>>> +fate-jxl-anim-demux-belgium: CMD = framecrc -i 
>>> $(TARGET_SAMPLES)/jxl/belgium.jxl -c copy
>>> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-lenna256
>>> +fate-jxl-anim-demux-lenna256: CMD = framecrc -i 
>>> $(TARGET_SAMPLES)/jxl/lenna-256.jxl -c copy
>>> +
>>> +FATE_JPEGXL_ANIM_DEMUX += $(FATE_JPEGXL_ANIM_DEMUX-yes)
>>> +
>>> +FATE_SAMPLES_FFMPEG-$(call FRAMECRC, JPEGXL_ANIM) += 
>>> $(FATE_JPEGXL_ANIM_DEMUX)
>>> +fate-jxl-anim-demux: $(FATE_JPEGXL_ANIM_DEMUX)
>>> diff --git a/tests/ref/fate/jxl-anim-demux-belgium 
>>> b/tests/ref/fate/jxl-anim-demux-belgium
>>> new file mode 100644
>>> index 0000000000..b2fe5035ac
>>> --- /dev/null
>>> +++ b/tests/ref/fate/jxl-anim-demux-belgium
>>> @@ -0,0 +1,6 @@
>>> +#tb 0: 1/25
>>> +#media_type 0: video
>>> +#codec_id 0: jpegxl
>>> +#dimensions 0: 768x512
>>> +#sar 0: 0/1
>>> +0,          0,          0,        1,       32, 0xa2930a20
>>> diff --git a/tests/ref/fate/jxl-anim-demux-icos4d 
>>> b/tests/ref/fate/jxl-anim-demux-icos4d
>>> new file mode 100644
>>> index 0000000000..eff6ff1f1b
>>> --- /dev/null
>>> +++ b/tests/ref/fate/jxl-anim-demux-icos4d
>>> @@ -0,0 +1,6 @@
>>> +#tb 0: 1/1000
>>> +#media_type 0: video
>>> +#codec_id 0: jpegxl
>>> +#dimensions 0: 48x48
>>> +#sar 0: 0/1
>>> +0,          0,          0,        0,    67898, 0x53b6516b
>>> diff --git a/tests/ref/fate/jxl-anim-demux-lenna256 
>>> b/tests/ref/fate/jxl-anim-demux-lenna256
>>> new file mode 100644
>>> index 0000000000..0bd286a451
>>> --- /dev/null
>>> +++ b/tests/ref/fate/jxl-anim-demux-lenna256
>>> @@ -0,0 +1,7 @@
>>> +#tb 0: 1/25
>>> +#media_type 0: video
>>> +#codec_id 0: jpegxl
>>> +#dimensions 0: 256x256
>>> +#sar 0: 0/1
>>> +0,          0,          0,        1,     4096, 0x2409e9e3
>>> +0,          1,          1,        1,     3992, 0x966dbfcb
>>
>> What this tells me is that the parser needs to do bitstream assembly 
>> after all. Image2 should not propagate a single image split in two 
>> packets like this, at the arbitrary limit of 4kb.
>>
>> Since this format seems to have actual delimiters 
>> (FF_JPEGXL_CODESTREAM_SIGNATURE_LE and 
>> FF_JPEGXL_CONTAINER_SIGNATURE_LE) and even buffer bytes with 
>> ff_jpegxl_collect_codestream_header(), you should then do the assembly 
>> in the parser, much like it's done for bmp, jpg and many other image 
>> formats.
>> The anim demuxer can remain as PARSE_HEADERS so it doesn't run the 
>> frame assembly code.
>>
> 
> The codestream signature is 0xFF 0x0A, which can and frequently will 
> occur unescaped in the middle of a valid file. Detecting the end of the 
> file is also a Hard Problem as it requires an entropy decoder, which I 
> believe you NAK'd for being overly complex for a parser in the first 
> iteration of this a few months ago.

Can you link that first iteration?

> 
> If I choose to assemble a bitstream here with the parser, what will end 
> up happening is that the entire sequence of all input frames will be one 
> large AVPacket, as detecting the end of the image is nontrivial. Is this 
> behavior desirable, compared to what image2dec does, which is emit 
> arbitrary 4k-sized packets?

What do you mean by all input frames? In the case of animated jpegxl, 
the anim demuxer would handle it, not image2. image2 should be used to 
read and output single frame images.

In any case, what would happen is that image2 will propagate packets of 
up to 4kb size, which the parser invoked in demux.c will then consume 
and not return anything until it has decided it has a complete coded 
image a decoder can then use to fully decode and output a frame, or a 
muxer encapsulate. Said complete coded image is output as a single 
buffer which is propagated to the caller as a single AVPacket.

> 
> If so, I can change it to assemble a packet, but I just want to make 
> sure that behavior is desirable over what I have here.
> 
> - Leo Izen
> _______________________________________________
> 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".
Leo Izen June 28, 2023, 12:33 a.m. UTC | #4 
On 6/27/23 19:46, James Almer wrote:
> On 6/27/2023 8:32 PM, Leo Izen wrote:
>> On 6/27/23 19:25, James Almer wrote:
>>> On 6/26/2023 12:49 PM, Leo Izen wrote:
>>>> Adds a fate test for the jpegxl_anim demuxer, that should allow testing
>>>> for true positives and false positives for animated jpegxl files. Note
>>>> that two of the test cases are not animated, in order to help sort out
>>>> false positives.
>>>>
>>>> Signed-off-by: <leo.izen@gmail.com>
>>>> ---
>>>>   tests/Makefile                         |  1 +
>>>>   tests/fate/jxl.mak                     | 16 ++++++++++++++++
>>>>   tests/ref/fate/jxl-anim-demux-belgium  |  6 ++++++
>>>>   tests/ref/fate/jxl-anim-demux-icos4d   |  6 ++++++
>>>>   tests/ref/fate/jxl-anim-demux-lenna256 |  7 +++++++
>>>>   tests/ref/fate/jxl-anim-demux-newton   |  6 ++++++
>>>>   6 files changed, 42 insertions(+)
>>>>   create mode 100644 tests/fate/jxl.mak
>>>>   create mode 100644 tests/ref/fate/jxl-anim-demux-belgium
>>>>   create mode 100644 tests/ref/fate/jxl-anim-demux-icos4d
>>>>   create mode 100644 tests/ref/fate/jxl-anim-demux-lenna256
>>>>   create mode 100644 tests/ref/fate/jxl-anim-demux-newton
>>>>
>>>> diff --git a/tests/Makefile b/tests/Makefile
>>>> index e09f30a0fc..7b80762e81 100644
>>>> --- a/tests/Makefile
>>>> +++ b/tests/Makefile
>>>> @@ -201,6 +201,7 @@ include $(SRC_PATH)/tests/fate/image.mak
>>>>   include $(SRC_PATH)/tests/fate/imf.mak
>>>>   include $(SRC_PATH)/tests/fate/indeo.mak
>>>>   include $(SRC_PATH)/tests/fate/jpeg2000.mak
>>>> +include $(SRC_PATH)/tests/fate/jxl.mak
>>>>   include $(SRC_PATH)/tests/fate/libavcodec.mak
>>>>   include $(SRC_PATH)/tests/fate/libavdevice.mak
>>>>   include $(SRC_PATH)/tests/fate/libavformat.mak
>>>> diff --git a/tests/fate/jxl.mak b/tests/fate/jxl.mak
>>>> new file mode 100644
>>>> index 0000000000..057d3be0e1
>>>> --- /dev/null
>>>> +++ b/tests/fate/jxl.mak
>>>> @@ -0,0 +1,16 @@
>>>> +# These two are animated JXL files
>>>> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-newton
>>>> +fate-jxl-anim-demux-newton: CMD = framecrc -i 
>>>> $(TARGET_SAMPLES)/jxl/newton.jxl -c copy
>>>> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-icos4d
>>>> +fate-jxl-anim-demux-icos4d: CMD = framecrc -i 
>>>> $(TARGET_SAMPLES)/jxl/icos4d.jxl -c copy
>>>> +
>>>> +# These two are not animated JXL. They are here to check false 
>>>> positives.
>>>> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-belgium
>>>> +fate-jxl-anim-demux-belgium: CMD = framecrc -i 
>>>> $(TARGET_SAMPLES)/jxl/belgium.jxl -c copy
>>>> +FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-lenna256
>>>> +fate-jxl-anim-demux-lenna256: CMD = framecrc -i 
>>>> $(TARGET_SAMPLES)/jxl/lenna-256.jxl -c copy
>>>> +
>>>> +FATE_JPEGXL_ANIM_DEMUX += $(FATE_JPEGXL_ANIM_DEMUX-yes)
>>>> +
>>>> +FATE_SAMPLES_FFMPEG-$(call FRAMECRC, JPEGXL_ANIM) += 
>>>> $(FATE_JPEGXL_ANIM_DEMUX)
>>>> +fate-jxl-anim-demux: $(FATE_JPEGXL_ANIM_DEMUX)
>>>> diff --git a/tests/ref/fate/jxl-anim-demux-belgium 
>>>> b/tests/ref/fate/jxl-anim-demux-belgium
>>>> new file mode 100644
>>>> index 0000000000..b2fe5035ac
>>>> --- /dev/null
>>>> +++ b/tests/ref/fate/jxl-anim-demux-belgium
>>>> @@ -0,0 +1,6 @@
>>>> +#tb 0: 1/25
>>>> +#media_type 0: video
>>>> +#codec_id 0: jpegxl
>>>> +#dimensions 0: 768x512
>>>> +#sar 0: 0/1
>>>> +0,          0,          0,        1,       32, 0xa2930a20
>>>> diff --git a/tests/ref/fate/jxl-anim-demux-icos4d 
>>>> b/tests/ref/fate/jxl-anim-demux-icos4d
>>>> new file mode 100644
>>>> index 0000000000..eff6ff1f1b
>>>> --- /dev/null
>>>> +++ b/tests/ref/fate/jxl-anim-demux-icos4d
>>>> @@ -0,0 +1,6 @@
>>>> +#tb 0: 1/1000
>>>> +#media_type 0: video
>>>> +#codec_id 0: jpegxl
>>>> +#dimensions 0: 48x48
>>>> +#sar 0: 0/1
>>>> +0,          0,          0,        0,    67898, 0x53b6516b
>>>> diff --git a/tests/ref/fate/jxl-anim-demux-lenna256 
>>>> b/tests/ref/fate/jxl-anim-demux-lenna256
>>>> new file mode 100644
>>>> index 0000000000..0bd286a451
>>>> --- /dev/null
>>>> +++ b/tests/ref/fate/jxl-anim-demux-lenna256
>>>> @@ -0,0 +1,7 @@
>>>> +#tb 0: 1/25
>>>> +#media_type 0: video
>>>> +#codec_id 0: jpegxl
>>>> +#dimensions 0: 256x256
>>>> +#sar 0: 0/1
>>>> +0,          0,          0,        1,     4096, 0x2409e9e3
>>>> +0,          1,          1,        1,     3992, 0x966dbfcb
>>>
>>> What this tells me is that the parser needs to do bitstream assembly 
>>> after all. Image2 should not propagate a single image split in two 
>>> packets like this, at the arbitrary limit of 4kb.
>>>
>>> Since this format seems to have actual delimiters 
>>> (FF_JPEGXL_CODESTREAM_SIGNATURE_LE and 
>>> FF_JPEGXL_CONTAINER_SIGNATURE_LE) and even buffer bytes with 
>>> ff_jpegxl_collect_codestream_header(), you should then do the 
>>> assembly in the parser, much like it's done for bmp, jpg and many 
>>> other image formats.
>>> The anim demuxer can remain as PARSE_HEADERS so it doesn't run the 
>>> frame assembly code.
>>>
>>
>> The codestream signature is 0xFF 0x0A, which can and frequently will 
>> occur unescaped in the middle of a valid file. Detecting the end of 
>> the file is also a Hard Problem as it requires an entropy decoder, 
>> which I believe you NAK'd for being overly complex for a parser in the 
>> first iteration of this a few months ago.
> 
> Can you link that first iteration?


https://patchwork.ffmpeg.org/project/ffmpeg/cover/20220401002006.44582-1-leo.izen@gmail.com/

> 
>>
>> If I choose to assemble a bitstream here with the parser, what will 
>> end up happening is that the entire sequence of all input frames will 
>> be one large AVPacket, as detecting the end of the image is 
>> nontrivial. Is this behavior desirable, compared to what image2dec 
>> does, which is emit arbitrary 4k-sized packets?
> 
> What do you mean by all input frames? In the case of animated jpegxl, 
> the anim demuxer would handle it, not image2. image2 should be used to 
> read and output single frame images.
> 
> In any case, what would happen is that image2 will propagate packets of 
> up to 4kb size, which the parser invoked in demux.c will then consume 
> and not return anything until it has decided it has a complete coded 
> image a decoder can then use to fully decode and output a frame, or a 
> muxer encapsulate. Said complete coded image is output as a single 
> buffer which is propagated to the caller as a single AVPacket.
> 
>>
>> If so, I can change it to assemble a packet, but I just want to make 
>> sure that behavior is desirable over what I have here.
>>
>> - Leo Izen
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel 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".
diff mbox series

Patch

diff --git a/tests/Makefile b/tests/Makefile
index e09f30a0fc..7b80762e81 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -201,6 +201,7 @@  include $(SRC_PATH)/tests/fate/image.mak
 include $(SRC_PATH)/tests/fate/imf.mak
 include $(SRC_PATH)/tests/fate/indeo.mak
 include $(SRC_PATH)/tests/fate/jpeg2000.mak
+include $(SRC_PATH)/tests/fate/jxl.mak
 include $(SRC_PATH)/tests/fate/libavcodec.mak
 include $(SRC_PATH)/tests/fate/libavdevice.mak
 include $(SRC_PATH)/tests/fate/libavformat.mak
diff --git a/tests/fate/jxl.mak b/tests/fate/jxl.mak
new file mode 100644
index 0000000000..057d3be0e1
--- /dev/null
+++ b/tests/fate/jxl.mak
@@ -0,0 +1,16 @@ 
+# These two are animated JXL files
+FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-newton
+fate-jxl-anim-demux-newton: CMD = framecrc -i $(TARGET_SAMPLES)/jxl/newton.jxl -c copy
+FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-icos4d
+fate-jxl-anim-demux-icos4d: CMD = framecrc -i $(TARGET_SAMPLES)/jxl/icos4d.jxl -c copy
+
+# These two are not animated JXL. They are here to check false positives.
+FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-belgium
+fate-jxl-anim-demux-belgium: CMD = framecrc -i $(TARGET_SAMPLES)/jxl/belgium.jxl -c copy
+FATE_JPEGXL_ANIM_DEMUX += fate-jxl-anim-demux-lenna256
+fate-jxl-anim-demux-lenna256: CMD = framecrc -i $(TARGET_SAMPLES)/jxl/lenna-256.jxl -c copy
+
+FATE_JPEGXL_ANIM_DEMUX += $(FATE_JPEGXL_ANIM_DEMUX-yes)
+
+FATE_SAMPLES_FFMPEG-$(call FRAMECRC, JPEGXL_ANIM) += $(FATE_JPEGXL_ANIM_DEMUX)
+fate-jxl-anim-demux: $(FATE_JPEGXL_ANIM_DEMUX)
diff --git a/tests/ref/fate/jxl-anim-demux-belgium b/tests/ref/fate/jxl-anim-demux-belgium
new file mode 100644
index 0000000000..b2fe5035ac
--- /dev/null
+++ b/tests/ref/fate/jxl-anim-demux-belgium
@@ -0,0 +1,6 @@ 
+#tb 0: 1/25
+#media_type 0: video
+#codec_id 0: jpegxl
+#dimensions 0: 768x512
+#sar 0: 0/1
+0,          0,          0,        1,       32, 0xa2930a20
diff --git a/tests/ref/fate/jxl-anim-demux-icos4d b/tests/ref/fate/jxl-anim-demux-icos4d
new file mode 100644
index 0000000000..eff6ff1f1b
--- /dev/null
+++ b/tests/ref/fate/jxl-anim-demux-icos4d
@@ -0,0 +1,6 @@ 
+#tb 0: 1/1000
+#media_type 0: video
+#codec_id 0: jpegxl
+#dimensions 0: 48x48
+#sar 0: 0/1
+0,          0,          0,        0,    67898, 0x53b6516b
diff --git a/tests/ref/fate/jxl-anim-demux-lenna256 b/tests/ref/fate/jxl-anim-demux-lenna256
new file mode 100644
index 0000000000..0bd286a451
--- /dev/null
+++ b/tests/ref/fate/jxl-anim-demux-lenna256
@@ -0,0 +1,7 @@ 
+#tb 0: 1/25
+#media_type 0: video
+#codec_id 0: jpegxl
+#dimensions 0: 256x256
+#sar 0: 0/1
+0,          0,          0,        1,     4096, 0x2409e9e3
+0,          1,          1,        1,     3992, 0x966dbfcb
diff --git a/tests/ref/fate/jxl-anim-demux-newton b/tests/ref/fate/jxl-anim-demux-newton
new file mode 100644
index 0000000000..6fcb85c41e
--- /dev/null
+++ b/tests/ref/fate/jxl-anim-demux-newton
@@ -0,0 +1,6 @@ 
+#tb 0: 1/1000
+#media_type 0: video
+#codec_id 0: jpegxl
+#dimensions 0: 128x96
+#sar 0: 0/1
+0,          0,          0,        0,    43376, 0xb2296182