Re: [PATCH] pickles: Add new pickle for jpeg

[Mohammad-Reza Nabipoor]

Hi Andreas.

Thank you for sharing the draft for this pickle.
Please find the comments below.


On Mon, Jan 29, 2024 at 06:19:20PM +0100, Andreas Klinger wrote:
> 2024-01-29  Andreas Klinger  <ak@it-klinger.de>
> 
>         * pickles/jpeg.pk: New pickle for jpeg files
> ---
> 
> This is also a RFC as there is a topic i'm not completely satisfied:
> 
> - When uncommenting 
>     byte[] image
>   in JPEG_Tag it takes a long time to use the pickle.


Yeah, that's a real problem.  One possible answer that we're thinking about
is "lazy" arrays that will not try to map all the elements of the array and
only map/poke/peek the content of the array when it is necessary.

>   Maybe there is a better way to define it.


The technique is to have some "marker" fields (like
`uint<8>[0] _marker : some_constraint;') to mark the boundaries and provide
some functions to get different view to the data when needed.
Of course, the data format should be "good enough" to enable you to find
the boundaries easily :)

ELF is an example of a good format that gives you a table in the beginning which
enables you easily find the boundaries of different entities.
Please see `Elf64_File' type definition in `elf-64.pk' in poke-elf pickle
(https://jemarch.net/poke-elf.html).


But these are "workarounds" till we implement the lazy arrays (or other
solutions).


> 
> - For my purposes i don't need the image but when submitting this pickle it
>   could be relevant to other users.
> 
> diff --git a/pickles/jpeg.pk b/pickles/jpeg.pk
> new file mode 100644
> index 00000000..ca07bf81
> --- /dev/null
> +++ b/pickles/jpeg.pk
> @@ -0,0 +1,114 @@
> +/* jpeg.pk - JPEG implementation for GNU poke */
> +
> +/* Copyright (C) 2024 Andreas Klinger */
> +
> +/* This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/* Implemented as specified in
> + * https://en.wikipedia.org/wiki/JPEG as well as
> + * https://de.wikipedia.org/wiki/JPEG_File_Interchange_Format  */
> +
> +/* Marker of JPEG pictures
> + * Array id 0 ("SOF0") starts at binary marker value 0xC0:
> + *   0xC0, 0xC1, ... , 0xC7
> + *   0xC8, 0xC1, ... , 0xCF
> + *   0xD0, 0xD1, ... , 0xD7
> + *   ...
> + *   0xF8, 0xF1, ... , 0xFF
> + */
> +var jpeg_marker =
> +  ["SOF0", "SOF1", "SOF2", "SOF3", "DHT", "SOF5", "SOF6", "SOF7",
> +   "JPG", "SOF9", "SOF10", "SOF11", "DAC", "SOF13", "SOF14", "SOF15",
> +   "-", "-", "-", "-", "-", "-", "-", "-",
> +   "SOI", "EOI", "SOS", "DQT", "-", "DRI", "-", "-",
> +   "APP0", "APP1", "APP2", "APP3", "APP4", "APP5", "APP6", "APP7",
> +   "APP8", "APP9", "APP10", "APP11", "APP12", "APP13", "APP14", "APP15",
> +   "-", "-", "-", "-", "-", "-", "-", "-",
> +   "-", "-", "-", "-", "-", "-", "COM", "-"];
> +
> +type JPEG_SOI =
> +  struct
> +  {
> +    byte ff = 0xFF;
> +    byte xx = 0xD8;
> +    method _print = void:
> +    {
> +      printf ("\n#<tag: %u16x\tsymbol: %s>",
> +                        ff:::xx, jpeg_marker[xx - 0xC0]);
> +    }
> +  };
> +
> +type JPEG_SOS =
> +  struct
> +  {
> +    byte ff = 0xFF;
> +    byte xx = 0xDA;
> +    method _print = void:
> +    {
> +      printf ("\n#<tag: %u16x\tsymbol: %s>",
> +                        ff:::xx, jpeg_marker[xx - 0xC0]);
> +    }
> +  };
> +
> +type JPEG_EOI =
> +  struct
> +  {
> +    byte ff = 0xFF;
> +    byte xx = 0xD9;
> +  };
> +
> +type JPEG_Segment =
> +  struct
> +  {
> +    byte ff == 0xFF;
> +    byte xx : xx != 0xD8 && xx != 0xDA && xx != 0xD9 && xx >= 0xC0 && xx < 
> 0xFF;
> +    uint16 size;
> +    char[size-2] data;


`size-2' is not a good idea because you cannot guarantee that `size' is always
`>= 2' and this happens:

```
(poke) var size = 0UH
(poke) size - 2
4294967294U
```

which is not easy to debug!


The other thing is all the "standard types" (things like `byte', `uint16', etc.)
are defined in `std-types.pk'.
These are very handy while developing/poking from the CLI.  But using them 
inside
pickles is not a good idea because loading of `std-types.pk' is optional (by
initializing the libpoke using
`pk_compiler_new_with_flags (&term_if, PK_F_NOSTDTYPES);').  Those libpoke
instances cannot load pickles and a real-world example of such is gdb 
integration
of libpoke.

So please use `uint<8>', `uint<16>' instead of `byte' and `uint16'.


> +    method _print = void:
> +    {
> +      printf ("\n#<tag: %u16x\tsymbol: %s\tsize: %u16d>",
> +                        ff:::xx, jpeg_marker[xx - 0xC0], size);
> +    }
> +  };
> +
> +fun JPEG_find_eoi = (byte[] image) int:
> +  {
> +    printf ("size: %u32d length: %u32d\n", image'size / 1#B, image'length);
> +    for (var i = image'length - 1; i > 1; i--)
> +      {
> +        var ff = image[i - 1];
> +        var xx = image[i];
> +        if (ff == 0xFF && xx == 0xD9)
> +          {
> +            return i;
> +          }
> +      }
> +    return 0;
> +  }
> +
> +type JPEG_Tag =
> +  struct
> +  {
> +    JPEG_SOI soi;
> +    JPEG_Segment[] segments;
> +    JPEG_SOS sos;
> +/*    byte[] image; */  /* could take some time for loading */
> +/*    var image_size = JPEG_find_eoi(image); */
> +/*    JPEG_EOI eoi; */


For a more idiomatic approach you can see `BER_Variable_Contents' type defined
in `asn1-ber.pk'.  You can find the detailed explanation of the technique
here: https://pokology.org/tips-and-tricks.html (section "1 Sequence of bytes
ended by two consecutive zero bytes").



Happy poking!