Re: [RFC, WIP, v4 08/11] media: vidtv: implement a PSI generator

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Wed, 6 May 2020 03:28:17 -0300
"Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu:

> >> +	/* Just a sanity check, should not really happen because we stuff
> >> +	 * the packet when we finish a section, i.e. when we write the crc at
> >> +	 * the end. But if this happens then we have messed up the logic
> >> +	 * somewhere.
> >> +	 */
> >> +	WARN_ON(args.new_psi_section && !aligned);  
> >  
> > Please use a ratelimited printk instead (here and on all similar cases
> > at the TS generator).
> >  
> > Also, I think that, on such case, the driver should be filling the
> > remaining frame with pad bytes. E. g.:
> >  
> > 	/*  
> > 	 * Assuming that vidtv_memset() args from patch 06/11 were changed  
> > 	 * according with this prototype:  
> > 	 */
> > 	size_t vidtv_memset(void *to, size_t to_offset, size_t to_size,
> > 			    u8 c, size_t len);
> >  
> >  
> > 	#define TS_FILL_BYTE 0xff
> >  
> > 	if (args.new_psi_section && !aligned) {
> > 		pr_warn_ratelimit("Warning: PSI not aligned. Re-aligning it\n");
> >  
> > 		vidtv_memset(args.dest_buf,
> > 			     args.dest_offset + nbytes_past_boundary,
> > 			     args.dest_buf_sz,
> > 			     TS_FILL_BYTE,		
> > 			     TS_PACKET_LEN - nbytes_past_boundary);
> > 		args.dest_offset += TS_PACKET_LEN - nbytes_past_boundary;
> > 		aligned = 1;
> > 		nbytes_past_boundary = 0;
> > 	}
> >    
> 
> Sure, that's fine then! Just to be clear this should not happen at all,
> because the writes should go through one of these four functions (IIRC!):
> 
> u32 vidtv_ts_null_write_into(struct null_packet_write_args args)
> u32 vidtv_ts_pcr_write_into(struct pcr_write_args args)
> 
> ...which will write a single packet at a time, thus leaving the buffer
> aligned if it was already aligned to begin with,
> 
> and
> 
> u32 vidtv_pes_write_into(struct pes_write_args args)
> static u32
> vidtv_psi_ts_psi_write_into(struct psi_write_args args)
> 
> ...which will pad when they finish writing a PES packet or a table
> section, respectively.
> 
> I left these warnings behind as a way to warn me if the padding logic
> itself is broken.

OK!

> > Please use a ratelimited printk instead (here and on all similar cases
> > at the TS generator).  
> 
> OK.
> 
> 
> 
> >> +static void vidtv_psi_desc_to_be(struct vidtv_psi_desc *desc)
> >> +{
> >> +	/*
> >> +	 * Convert descriptor endianness to big-endian on a field-by-field
> >> basis
> >> +	 * where applicable
> >> +	 */
> >> +
> >> +	switch (desc->type) {
> >> +	/* nothing do do */
> >> +	case SERVICE_DESCRIPTOR:
> >> +		break;
> >> +	case REGISTRATION_DESCRIPTOR:
> >> +		cpu_to_be32s(&((struct vidtv_psi_desc_registration *)
> >> +			     desc)->format_identifier);
> >> +		pr_alert("%s: descriptor type %d found\n",
> >> +			 __func__,
> >> +			 desc->type);
> >> +		pr_alert("%s: change 'additional_info' endianness before
> >> calling\n",
> >> +			 __func__);  
> >  
> > The above pr_alert() calls sound weird. Why are you unconditionally
> > calling it (and still doing the BE conversion) for all
> > REGISTRATION_DESCRIPTOR types?  
> 
> To be honest, I did not know what to do. Here's what 13818-1 has to say
> about registration descriptors:
> 
> >2.6.9
> > Semantic definition of fields in registration descriptor
> >format_identifier – The format_identifier is a 32-bit value obtained
> >from a Registration Authority as designated by
> >ISO/IEC JTC 1/SC 29.
> >additional_identification_info – The meaning of
> >additional_identification_info bytes, if any, are defined by the
> >assignee of that format_identifier, and once defined they shall not change.  
> 
> So I took the cue from libdvbv5 and defined the following struct for it,
> with a flexible array member at the end:

André (who re-wrote the libdvbv5 parsers to be more generic)
preferred the approach of keeping the structs in CPU-endian, as it
makes easier from application PoV, as the conversion happens only once
at the library.

That's not the case here. We can fill the structs in big endian,
converting to CPU-endian only on the few places we may need to read
back from it.

> 
> struct vidtv_psi_desc_registration {
> 	u8 type;
> 	u8 length;
> 	struct vidtv_psi_desc *next;
> 
> 	/*
> 	 * The format_identifier is a 32-bit value obtained from a Registration
> 	 * Authority as designated by ISO/IEC JTC 1/SC 29.
> 	 */
> 	u32 format_identifier;
> 	/*
> 	 * The meaning of additional_identification_info bytes, if any, are
> 	 * defined by the assignee of that format_identifier, and once defined
> 	 * they shall not change.
> 	 */
> 	u8 additional_identification_info[];
> } __packed
> 
> 
> As you know, I was changing the endianness from host to BE before
> serializing and then changing back from BE to host. Given the struct
> definition above, there was no way to do this to the
> 'additional_identification_info' member, since we do not know its layout.
> 
> If we go with your approach instead (i.e. using __be16, __be32...etc)
> then I think we can remove these two functions (and more)

Yep. 

Thanks,
Mauro