[DVDnav-discuss] [PATCH] Patch 2/5: EXTFINFO and Filetype 250

[Dominik 'Rathann' Mierzejewski]

On Sunday, 27 September 2009 at 01:49, Jorgen Lundman wrote:
> 
> >Could you resend the whole UDF patchset rediffed (if necessary) against
> >current SVN? Nico, please review it when you have some time. I seem to
> >remember that at least one patch was already OK'd by Nico.
> >
> 
> I am dealing directly with Hovland to re-attempt my patches. I have sent 
> him the first one to fix the AD chains. If that is accepted, I will do the 
> 2nd one.

Some comments below.

[...]
> Index: dvd_udf.c
> ===================================================================
> --- dvd_udf.c	(revision 1181)
> +++ dvd_udf.c	(working copy)
> @@ -40,6 +40,7 @@
>  #include "dvdread/dvd_reader.h"
>  #include "dvdread/dvd_udf.h"
>  
> +
>  /* Private but located in/shared with dvd_reader.c */
>  extern int UDFReadBlocksRaw( dvd_reader_t *device, uint32_t lb_number,
>                               size_t block_count, unsigned char *data,

Cosmetics.

> @@ -782,7 +807,80 @@
>    return part->valid;
>  }
>  
> -uint32_t UDFFindFile( dvd_reader_t *device, char *filename,
> +
> +/*
> + * Release a UDF file descriptor
> + */
> +void UDFFreeFile(dvd_reader_t *device, udf_file_t *File)
> +{
> +  free(File);
> +}

I think the wrapping of free is unnecessary here, especially when
the device parameter is unused. dvd_reader.c changes would need
to be adjusted accordingly as well.

> +/*
> + * The API users will refer to block 0 as start of file and going up
> + * we need to convert that to actual disk block; partition_start +
> + * file_start + offset, but keep in mind that file_start is chained,
> + * and not contiguous.
> + *
> + * We return "0" as error, since a File can not start at physical block 0
> + *
> + * It is perhaps unfortunate that Location is in blocks, but Length is
> + * in bytes..? Can bytes be uneven blocksize in the middle of a chain?
> + *
> + */
> +uint32_t UDFFileBlockPos(udf_file_t *File, uint32_t file_block)
> +{
> +  uint32_t result, i, offset;
> +
> +  if (!File) return 0;
> +
> +  /* Look through the chain to see where this block would belong. */
> +  for (i = 0, offset = 0;
> +       i < File->num_AD;
> +       i++) {

Ugly indentation.

> +
> +    /* Is "file_block" inside this chain? Then use this chain. */
> +    if (file_block < (offset +
> +                      (File->AD_chain[i].Length /  DVD_VIDEO_LB_LEN)))
> +      break;
> +
> +    offset += (File->AD_chain[i].Length /  DVD_VIDEO_LB_LEN);
> +
> +  }
> +
> +  /* If it is not defined in the AD chains, we should return an error, but
> +   * in the interest of being backward (in)compatible with the original
> +   * libdvdread, we revert back to the traditional view that UDF file are
> +   * one contiguous long chain, in case some API software out there relies on
> +   * this incorrect behavior.
> +   */
> +  if (i >= File->num_AD)
> +    i = 0;
> +
> +
> +  result = File->Partition_Start
> +    + File->AD_chain[i].Location
> +    + file_block
> +    - offset;

Unnecessary line breaks?

Regards,
R.

-- 
Fedora http://fedoraproject.org/wiki/User:Rathann
RPMFusion http://rpmfusion.org | MPlayer http://mplayerhq.hu
"Faith manages."
        -- Delenn to Lennier in Babylon 5:"Confessions and Lamentations"