OpenCores

JPEG Encoder :: Bugtracker

Issue List
Quantization header missing update #12
Open ooterness opened this issue about 6 years agoBug
ooterness commented about 6 years ago

In the latest SVN revision, there is a potential to corrupt the header data stored in JFIFGen. Specifically, the recommended initialization procedure is to write the image size, followed by the two quantization tables. However, the JFIFGen state machine is busy for five clock cycles after updating the image size, while it stores the new size information. If the first quantization table entry is written immediately afterward, then the state machine will still be busy when the new data arrives, and the update will be lost. The result is an incorrect entry in the final JPEG header.

My proposed fix is to prioritize incoming quantization table data, and write the updated size information while the memory is otherwise idle. See below:

Change #1: Shorten size_wr_cnt from 3 bits to 2 bits signal size_wr_cnt : unsigned(1 downto 0);

Change #2: Prioritize updated size information p_host_wr : process(CLK, RST) begin if RST = '1' then size_wr_cnt <= (others => '0'); size_wr <= '0'; hr_we <= '0'; hr_data <= (others => '0'); hr_waddr <= (others => '0'); elsif CLK'event and CLK = '1' then hr_we <= '0';

  -- Note the fact that size has updated.  Write the four bytes
  -- during idle periods, so we don't accidentally clobber changes
  -- to the quantization tables, etc.
  if image_size_reg_wr = '1' then
    size_wr_cnt <= (others => '0');
    size_wr     <= '1';
  end if;
  
  -- Write any quantization table updates immediately.
  if qwren = '1' then
    -- luminance table select
    if qwaddr(6) = '0' then
      hr_waddr <= std_logic_vector
                    ( resize(unsigned(qwaddr(5 downto 0)),hr_waddr'length) + 
                      to_unsigned(C_QLUM_BASE,hr_waddr'length));
    else
      -- chrominance table select
      hr_waddr <= std_logic_vector
                    ( resize(unsigned(qwaddr(5 downto 0)),hr_waddr'length) + 
                      to_unsigned(C_QCHR_BASE,hr_waddr'length));
    end if;
    hr_we   <= '1';
    hr_data <= qwdata;
  -- Write the new image size when otherwise idle.
  elsif size_wr = '1' then
    case size_wr_cnt is
      -- height H byte
      when "00" =>
        hr_data  <= image_size_reg(15 downto 8);
        hr_waddr <= std_logic_vector(to_unsigned(C_SIZE_Y_H,hr_waddr'length));
      -- height L byte
      when "01" =>
        hr_data <= image_size_reg(7 downto 0);
        hr_waddr <= std_logic_vector(to_unsigned(C_SIZE_Y_L,hr_waddr'length));
      -- width H byte
      when "10" =>
        hr_data <= image_size_reg(31 downto 24);
        hr_waddr <= std_logic_vector(to_unsigned(C_SIZE_X_H,hr_waddr'length));
      -- width L byte
      when "11" =>
        hr_data <= image_size_reg(23 downto 16);
        hr_waddr <= std_logic_vector(to_unsigned(C_SIZE_X_L,hr_waddr'length));
        size_wr  <= '0';  -- Last byte --> done.
      when others =>
        null;
    end case;
    size_wr_cnt <= size_wr_cnt + 1;
    hr_we <= '1';
  end if;
end if;

end process;

This is just long enough that it can overlap with the first quantization table entry, and the updated table entry is lost. If the new quantization table differs from the

If This is enough time to receive the first entry in a new quantization table. a new size i

If a quantization table entry is written immediately after the image size is changed, then the JFIFGen RAM may miss the first entry or tw

ooterness commented about 6 years ago

Source code reformatted with HTML escape characters:

  p_host_wr : process(CLK, RST)   begin     if RST = '1' then       size_wr_cnt %lt;= (others =%gt; '0');       size_wr     %lt;= '0';       hr_we       %lt;= '0';       hr_data     %lt;= (others =%gt; '0');       hr_waddr    %lt;= (others =%gt; '0');     elsif CLK'event and CLK = '1' then       hr_we %lt;= '0';            -- Note the fact that size has updated.  Write the four bytes       -- during idle periods, so we don't accidentally clobber changes       -- to the quantization tables, etc.       if image_size_reg_wr = '1' then         size_wr_cnt %lt;= (others =%gt; '0');         size_wr     %lt;= '1';       end if;              -- Write any quantization table updates immediately.       if qwren = '1' then         -- luminance table select         if qwaddr(6) = '0' then           hr_waddr %lt;= std_logic_vector                         ( resize(unsigned(qwaddr(5 downto 0)),hr_waddr'length) +                            to_unsigned(C_QLUM_BASE,hr_waddr'length));         else           -- chrominance table select           hr_waddr %lt;= std_logic_vector                         ( resize(unsigned(qwaddr(5 downto 0)),hr_waddr'length) +                            to_unsigned(C_QCHR_BASE,hr_waddr'length));         end if;         hr_we   %lt;= '1';         hr_data %lt;= qwdata;       -- Write the new image size when otherwise idle.       elsif size_wr = '1' then         case size_wr_cnt is           -- height H byte           when "00" =%gt;             hr_data  %lt;= image_size_reg(15 downto 8);             hr_waddr %lt;= std_logic_vector(to_unsigned(C_SIZE_Y_H,hr_waddr'length));           -- height L byte           when "01" =%gt;             hr_data %lt;= image_size_reg(7 downto 0);             hr_waddr %lt;= std_logic_vector(to_unsigned(C_SIZE_Y_L,hr_waddr'length));           -- width H byte           when "10" =%gt;             hr_data %lt;= image_size_reg(31 downto 24);             hr_waddr %lt;= std_logic_vector(to_unsigned(C_SIZE_X_H,hr_waddr'length));           -- width L byte           when "11" =%gt;             hr_data %lt;= image_size_reg(23 downto 16);             hr_waddr %lt;= std_logic_vector(to_unsigned(C_SIZE_X_L,hr_waddr'length));             size_wr  %lt;= '0';  -- Last byte --%gt; done.           when others =%gt;             null;         end case;         size_wr_cnt %lt;= size_wr_cnt + 1;         hr_we %lt;= '1';       end if;     end if;   end process;

ooterness commented about 6 years ago

Reformatting, try #3:

  p_host_wr : process(CLK, RST)   begin     if RST = '1' then       size_wr_cnt <= (others =%gt; '0');       size_wr     <= '0';       hr_we       <= '0';       hr_data     <= (others =%gt; '0');       hr_waddr    <= (others =%gt; '0');     elsif CLK'event and CLK = '1' then       hr_we <= '0';            -- Note the fact that size has updated.  Write the four bytes       -- during idle periods, so we don't accidentally clobber changes       -- to the quantization tables, etc.       if image_size_reg_wr = '1' then         size_wr_cnt <= (others =%gt; '0');         size_wr     <= '1';       end if;              -- Write any quantization table updates immediately.       if qwren = '1' then         -- luminance table select         if qwaddr(6) = '0' then           hr_waddr <= std_logic_vector                         ( resize(unsigned(qwaddr(5 downto 0)),hr_waddr'length) +                            to_unsigned(C_QLUM_BASE,hr_waddr'length));         else           -- chrominance table select           hr_waddr <= std_logic_vector                         ( resize(unsigned(qwaddr(5 downto 0)),hr_waddr'length) +                            to_unsigned(C_QCHR_BASE,hr_waddr'length));         end if;         hr_we   <= '1';         hr_data <= qwdata;       -- Write the new image size when otherwise idle.       elsif size_wr = '1' then         case size_wr_cnt is           -- height H byte           when "00" =%gt;             hr_data  <= image_size_reg(15 downto 8);             hr_waddr <= std_logic_vector(to_unsigned(C_SIZE_Y_H,hr_waddr'length));           -- height L byte           when "01" =%gt;             hr_data <= image_size_reg(7 downto 0);             hr_waddr <= std_logic_vector(to_unsigned(C_SIZE_Y_L,hr_waddr'length));           -- width H byte           when "10" =%gt;             hr_data <= image_size_reg(31 downto 24);             hr_waddr <= std_logic_vector(to_unsigned(C_SIZE_X_H,hr_waddr'length));           -- width L byte           when "11" =%gt;             hr_data <= image_size_reg(23 downto 16);             hr_waddr <= std_logic_vector(to_unsigned(C_SIZE_X_L,hr_waddr'length));             size_wr  <= '0';  -- Last byte --%gt; done.           when others =%gt;             null;         end case;         size_wr_cnt <= size_wr_cnt + 1;         hr_we <= '1';       end if;     end if;   end process;