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

[Leo Izen <leo.izen@gmail.com>]

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".
_______________________________________________
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".