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

[Jose E. Marchesi]

> 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!

But you can use

  uint16 size : size >= 2;
  char[size >= 2 ? size - 2 : 0] data;

The conditional expression in the array size is to allow non-strict
mappings @!.

> 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!