Adding RLE-core module for JPEG encoder #2

ishan98 · 2017-08-20T06:33:01Z

This includes the addition of RLE module to the JPEG encoder with Test Bench :

RLE module :
The module takes an input_data and generate an output compressing all the zeros within two non-zero values in the form of runlength, which is used to compress the image.
RLEmain.py :
Connect the input to both the RLEcore and the Entrophycoder.
RLEcore.py :
Takes the input and calculate the runlength that is number of zeros between two non-zero values.
Entrophycoder.py :
Takes the input of the non-zero amplitude and calculate the number of bits required to store that amplitude.

Test Bench :
new_rlemain_tb.py
new_entrophycoder_tb.py
new_rlecore_tb.py
This three modules are made for the testing of the RLEmain, Entrophycoder and RLEcore respectively.

Common.py :
Added input_data for testing the RLE module and various functions such as setdata() to get the input and output in the required format.

After adding the above changes, the resultant repository contains the working model of the RLE module along with the test suite.

mithro · 2017-08-20T07:52:21Z

litejpeg/core/rle/entropycoder.py

+
+from litejpeg.core.common import *
+
+"""


This comment should be the first thing in the file. Can you move it there?

mithro · 2017-08-20T07:53:01Z

litejpeg/core/rle/entropycoder.py

+    Parameters:
+    -----------
+    sink : Get the input from the other modules.
+           The input is the modulus of the input.


The input is the modulus of the input? I'm not sure what that means?

mithro · 2017-08-20T07:54:20Z

litejpeg/core/rle/entropycoder.py

+
+        # For calculating the size.
+
+        # Storing the size in the input_data.


Your comments should concentrate on why you are doing something. The code explains how you are doing it.

For example, why do you want to iterate over different values from 1 to 12? Any python programmer understands that for i in range(12) iterates over the values 0->11.

mithro · 2017-08-20T07:54:52Z

litejpeg/core/rle/entropycoder.py

+    This module will connect the Entropycoder datapath with the input
+    and output either from other modules or from the Test Benches.
+    The input is been taken from the sink and source and is been transferred to
+    the Entropycoder datapath by using read and write count.


But what does the Entropycoder actually do?

mithro · 2017-08-20T07:55:00Z

litejpeg/core/rle/entropycoder.py

+        self.comb += source.data.eq(size)
+
+
+class Entropycoder(PipelinedActor, Module):


EntropyCoder

mithro · 2017-08-20T07:58:15Z

litejpeg/core/rle/rlecore.py

+        ]
+
+
+class Runlength(PipelinedActor, Module):


RunLength

mithro · 2017-08-20T08:00:11Z

test/common.py

+    testing.
+    """
+    def __init__(self):


Where do these values come from? What are they for?

Why is red_pixels_1, self.red_pixels_1 while everything else is not assigned to self?

mithro · 2017-08-20T08:00:33Z

test/common.py

+        for i in range(64):
+            temp=self.data[i]
+            Amplitude = temp%4096


amplitude

mithro · 2017-08-20T08:01:04Z

test/common.py

+        for i in range(64):
+            temp=self.data[i]
+            Amplitude = temp%4096


These numbers seem important. Why are you doing a %4096 and a %16 below.

mithro · 2017-08-20T08:02:04Z

test/new_rlemain_tb.py

@@ -0,0 +1,66 @@
+# This is the module for testing the RLEmain.
+
+# !/usr/bin/env python3


# !/usr/bin/env python3 must be the first line in the file.

mithro

Another round of comments. You are getting closer, I think we can probably get this merged today!

mithro · 2017-08-20T09:37:43Z

litejpeg/core/rle/entropycoder.py

+
+    Parameters:
+    -----------
+    sink : Get the input from the other modules.


Why does it want the input from the other modules?

I should be able to understand what the other module this is being connected and why is that a separate module.

mithro · 2017-08-20T09:38:13Z

litejpeg/core/rle/entropycoder.py

+    -----------
+    sink : Get the input from the other modules.
+    source : Give the output to the other modules.
+    input_data : modulus of input of the matrix.


What matrix? I assume the matrix is coming from somewhere else?

mithro · 2017-08-20T09:40:41Z

litejpeg/core/rle/entropycoder.py

+    It contains the steps for the Entropycoder to calculate the bits for the
+    Amplitude to store.
+
+    Parameters:


These are actually Attributes not Parameters.

Parameters is pretty much a synonym for "argument". For example def foo(a, b) has 2 parameters a & b.

mithro · 2017-08-20T09:40:58Z

litejpeg/core/rle/entropycoder.py

+    """
+    EntropyDatapath :
+    ------------------
+    It contains the steps for the Entropycoder to calculate the bits for the


Entroycoder case is wrong.

mithro · 2017-08-20T09:44:15Z

litejpeg/core/rle/entropycoder.py

+        write_fsm.act("WRITE_INPUT",
+                      sink.ready.eq(1),
+                      If(sink.valid,
+                         If(write_count == 63,


If(write_count == (BLOCK_COUNT-1), and then define BLOCK_COUNT to be 64 (and use it elsewhere).

mithro · 2017-08-20T10:01:23Z

litejpeg/core/rle/rlemain.py

+Parameters:
+-----------
+sink : 12 bits
+       Recieves an input of 12 bits either from test bench or from


What to the 12 bits of data represent?

mithro · 2017-08-20T10:01:33Z

litejpeg/core/rle/rlemain.py

+sink : 12 bits
+       Recieves an input of 12 bits either from test bench or from
+       other modules.
+source : 21 bits


What do the 21 bits represent?

mithro · 2017-08-20T10:01:56Z

litejpeg/core/rle/rlemain.py

+datapath_latency = 3
+
+
+class RLEmain(PipelinedActor, Module):


RLEmain should be RLEMain right?

mithro · 2017-08-20T10:04:22Z

test/common.py

+    These class is been created in order to store the value or the matrix which
+    are been used to test the RLE module.
+    The matrix are from github repositories for the purpose of input for


Which github repositories? There are plenty out there... How do I check that you haven't made a mistake in the matrixes?

mithro · 2017-08-20T10:05:25Z

test/common.py

+            # number therefore in order to extracts the two we take the modulus
+            # by 4096 to get last 12 bits and than shift and than again extracts
+            # the next 4 representing runlength.


This doesn't explain why we want to modulus by 4096? Is this the size of the data? Is it got something to do with runlength?

mithro

Almost there for merging this pull request!

mithro · 2017-08-27T06:56:52Z

litejpeg/core/rle/entropycoder.py

+    -----------
+    sink : Get the input from the Entrophycoder.
+    source : Give the output to the Entrophycoder.
+    input_data : for temporary storing the value of input, used


input_data / get_data / size are all not attributes. Attributes are things stored on self

mithro · 2017-08-27T06:59:48Z

litejpeg/core/rle/entropycoder.py

+    EntropyDatapath :
+    ------------------
+    It contains the steps for the EntropyCoder to calculate the bits for the
+    Amplitude to store.


Why is amplitude capitalized here?

A better description would be something like Calculate the bit value for the amplitude using EntropyCoder.

mithro · 2017-08-27T07:02:46Z

litejpeg/core/rle/entropycoder.py

+
+    Attributes:
+    -----------
+    sink : Get the input from the Entrophycoder.


Two things here;

Capitalization should always match. If the class is called EntrophyCoder you should always refer to it as EntrophyCoder never entrophyCoder or Entrophycoder.

"Get the input from the Entrophycoder." is not informative. It should be something like "Accepts data which is encoded in huffman entropy foo bad, such as the output from the EntrophyCoder class." -- The difference is that it tells me what the data is.

mithro · 2017-08-27T07:05:08Z

litejpeg/core/rle/entropycoder.py

+        # number of bits required to store the input_data.
+        for i in range(12):
+            self.sync += get_data[i].eq(input_data >> i)
+        for i in range(12):


for i in range(11, -1, -1):

I don't not able to understand this one.

http://pythoncentral.io/pythons-range-function-explained/

mithro · 2017-08-27T07:05:58Z

litejpeg/core/rle/entropycoder.py

+    the amplitude.
+
+    Attributes :
+    ------------


The indenting of the below things in a bit weird...

mithro · 2017-08-27T07:09:54Z

litejpeg/core/rle/rlecore.py

+        self.source = source = stream.Endpoint(
+                                   EndpointDescription(block_layout(17)))
+
+        # Adding PipelineActor to provide additional clock for the module.


Why do you need an additional clock?

mithro · 2017-08-27T07:10:42Z

litejpeg/core/rle/rlemain.py

+    RLE core as the output will contain the amplitude in the last 12 bits along
+    with the number of bits to store the amplitude as the next 4 bits and
+    finally the RunLength for the next 4 bits.
+    All these information is been synchronized once by the RLEmain and than


mithro · 2017-08-27T07:11:51Z

test/common.py

+    """
+    These class is been created in order to store the value or the matrix which
+    are been used to test the RLE module.


This class stores the value of the matrixes used to test the RLE module. These matrixes were taken from [`rle_test_inputs.py` in cfelton's test_jpeg code](https://github.com/cfelton/test_jpeg/blob/master/test/rle_test_inputs.py). The matrix is an example of what the quantization module might produce.

mithro · 2017-08-27T07:15:50Z

test/common.py

+        self.data = data
+        for i in range(64):
+            temp=self.data[i]


The data we get contains amplitude and run length as a single number. In order to extract the values we use mod 4096 to get the last 12bits and then shift to extract the next 4 representing the run length.

mithro · 2017-08-27T07:19:58Z

test/new_rlecore_tb.py

+input to the RLE core and the output is been printed and compared
+with the one from the reference modules.
+In this way the result from the RLEcore is been verified.
+"""


This module takes a matrix containing 64 blocks of XXXX and verifies the RLECore produces the same output as the reference data.

mithro · 2017-08-28T03:30:42Z

litejpeg/core/rle/rlecore.py

+        # To keep track over which value of the matrix is under process.
+        write_count = Signal(6)
+
+        # For tracking the data adress.


BTW For migen we normally wrap the code like this;

self.sync += \ If(write_clear, write_count.eq(0) ).Elif(write_inc, write_count.eq(write_count + 1) )

IE

If(<condition>, <4 spaces>statement ).Elif(<condition>, <4 spaces>statement1, <4 spaces>statement2, ).Else( <4 spaces>statement1, <4 spaces>statement2, )

DONE to most of the places.

mithro

LGTM!

Merging.

ishan98 added 6 commits July 14, 2017 14:49

Adding RLE core

4ba2f02

Rearranging RLE module

fb063a4

Adding RLEMain and Entrophycoder

a51c88f

Minor Spell correction

18ae9f3

Modifying Runlength

c078b99

Adding Indentaion

39e2e68

mithro reviewed Aug 20, 2017

View reviewed changes

Updating RLE

13932da

mithro reviewed Aug 20, 2017

View reviewed changes

ishan98 added 2 commits August 20, 2017 17:47

Updating RLE

8b4f6a0

Upadate RLE v2

912ad02

mithro reviewed Aug 27, 2017

View reviewed changes

ishan98 added 2 commits August 27, 2017 14:36

Updated RLEv3

e6ad7ad

Updating RLEv4

5edda6b

mithro reviewed Aug 28, 2017

View reviewed changes

Updating v5

4107904

mithro approved these changes Aug 31, 2017

View reviewed changes

mithro merged commit 2bd67e5 into litex-hub:master Aug 31, 2017


		# For calculating the size.

		# Storing the size in the input_data.

		self.comb += source.data.eq(size)


		class Entropycoder(PipelinedActor, Module):

		@@ -0,0 +1,66 @@
		# This is the module for testing the RLEmain.

		# !/usr/bin/env python3

Adding RLE-core module for JPEG encoder #2

Adding RLE-core module for JPEG encoder #2

Conversation

ishan98 commented Aug 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mithro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mithro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mithro left a comment

Choose a reason for hiding this comment