Re: [PATCH i-g-t 1/3] lib/igt_chamelium: Add chamelium_frame_match_or_dump_frame_pair()

["Sharma, Swati2" <swati2.sharma@intel.com>]

Hi Chaitanya,
Thankyou for the review. Please find my comments inline.

On 26-03-2025 05:53 pm, Borah, Chaitanya Kumar wrote:
> Hello Swati,
>
>> -----Original Message-----
>> From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of Swati
>> Sharma
>> Sent: Monday, March 24, 2025 2:40 PM
>> To: igt-dev@lists.freedesktop.org
>> Cc: Sharma, Swati2 <swati2.sharma@intel.com>
>> Subject: [PATCH i-g-t 1/3] lib/igt_chamelium: Add
>> chamelium_frame_match_or_dump_frame_pair()
>>
>> Add chamelium_frame_match_or_dump_frame_pair() to compare frame
>> dumps captured from chamelium.
>>
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>> ---
>>   lib/igt_chamelium.c | 60
>> +++++++++++++++++++++++++++++++++++++++++++++
>>   lib/igt_chamelium.h |  6 +++++
>>   2 files changed, 66 insertions(+)
>>
>> diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c index
>> 620fbbf7d..44a169b29 100644
>> --- a/lib/igt_chamelium.c
>> +++ b/lib/igt_chamelium.c
>> @@ -2070,6 +2070,66 @@ bool chamelium_frame_match_or_dump(struct
>> chamelium *chamelium,
>>   	return match;
>>   }
>>
>> +/**
>> + * chamelium_frame_match_or_dump:
>> + * @chamelium: The chamelium instance the frame dump belongs to
>> + * @frame0: The chamelium reference frame dump to match
>> + * @frame1: The chamelium capture frame dump to match
>> + * @reference_crc: CRC of chamelium reference frame
>> + * @check: the type of frame matching check to use
>> + *
>> + * Returns bool that the provided captured frame matches the reference
>> + * frame from the framebuffer. If they do not, this saves the reference
>> + * and captured frames to a png file.
>> + */
>> +bool chamelium_frame_match_or_dump_frame_pair(struct chamelium
>> *chamelium,
>> +					      struct chamelium_port *port,
>> +					      const struct
>> chamelium_frame_dump *frame0,
>> +					      const struct
>> chamelium_frame_dump *frame1,
>> +					      igt_crc_t *reference_crc,
> The asymmetry in the arguments (reference_crc and not capture_crc) is bother me a bit.
hmm..chamelium can compute CRC of the last captured frame, thats why 
computed ref CRC and passed it as an
argument. As far as symmetry is concerned; will compute CRC of both ref 
and capture frame in test and pass as
an argument. Will it work?
>
> ...
>
>> +					      enum chamelium_check check)
>> +{
>> +	cairo_surface_t *reference;
>> +	cairo_surface_t *capture;
>> +	igt_crc_t *capture_crc;
>> +	bool match;
>> +
>> +	/* Grab the captured reference frame from chamelium */
>> +	reference = convert_frame_dump_argb32(frame0);
>> +
>> +	/* Grab the captured frame from chamelium */
>> +	capture = convert_frame_dump_argb32(frame1);
>> +
>> +	switch (check) {
>> +	case CHAMELIUM_CHECK_ANALOG:
>> +		match = igt_check_analog_frame_match(reference, capture);
>> +		break;
>> +	case CHAMELIUM_CHECK_CHECKERBOARD:
>> +		match = igt_check_checkerboard_frame_match(reference,
>> capture);
>> +		break;
>> +	default:
>> +		igt_assert(false);
>> +	}
>> +
>> +	if (!match && igt_frame_dump_is_enabled()) {
>> +		/* Get the captured frame CRC from the Chamelium. */
>> +		capture_crc = chamelium_get_crc_for_area(chamelium, port,
>> 0, 0,
>> +							 0, 0);
> Does this mean that chamelium should be displaying the image when this comparison is done?
> If so, It will defeat the purpose of using the frame dump for comparison.
Frame dump comparison is already done before. Here we are just computing 
CRCs.
>
>
>> +		igt_assert(capture_crc);
>> +
>> +		compared_frames_dump(reference, capture, reference_crc,
>> +				     capture_crc);
> Since compared_frames_dump() calculates the crc if not passed by the caller we should let it.
> That way we can remove the need of passing the reference_crc to this helper. (and also not calculate capture_crc)
Like mentioned before, chamelium captures CRC of last captured frame. 
SO, in test we need to compute CRC just after
capture and then pass both ref and capture CRC as arg.
>
> Regards
>
> Chaitanya
>
>
>> +
>> +		free(reference_crc);
>> +		free(capture_crc);
>> +	}
>> +
>> +	cairo_surface_destroy(reference);
>> +	cairo_surface_destroy(capture);
>> +
>> +	return match;
>> +}
>> +
>>   /**
>>    * chamelium_analog_frame_crop:
>>    * @chamelium: The Chamelium instance to use diff --git
>> a/lib/igt_chamelium.h b/lib/igt_chamelium.h index d979de4a2..140d52c95
>> 100644
>> --- a/lib/igt_chamelium.h
>> +++ b/lib/igt_chamelium.h
>> @@ -257,6 +257,12 @@ bool chamelium_frame_match_or_dump(struct
>> chamelium *chamelium,
>>   				   const struct chamelium_frame_dump
>> *frame,
>>   				   struct igt_fb *fb,
>>   				   enum chamelium_check check);
>> +bool chamelium_frame_match_or_dump_frame_pair(struct chamelium
>> *chamelium,
>> +					      struct chamelium_port *port,
>> +					      const struct
>> chamelium_frame_dump *frame0,
>> +					      const struct
>> chamelium_frame_dump *frame1,
>> +					      igt_crc_t *reference_crc,
>> +					      enum chamelium_check check);
>>   void chamelium_crop_analog_frame(struct chamelium_frame_dump *dump,
>> int width,
>>   				 int height);
>>   void chamelium_destroy_frame_dump(struct chamelium_frame_dump
>> *dump);
>> --
>> 2.25.1