1193119 - Refactor the nsGIFDecoder2::WriteInternal() method

Status: RESOLVED DUPLICATE of bug 1204392

(Core :: Graphics: ImageLib, defect)

Description

[Mihai Volmer [:mihaivolmer]]

The nsGIFDecoder2::WriteInternal() method is more than 500 lines long. Thus, it is very hard to follow and debug. Some refactoring is needed and the first step is separating the metadata decoding code from this method.
Moreover, a separate method for decoding the metadata could be useful in other situations.

Comment 1

[Mihai Volmer [:mihaivolmer]]

Attachment: Separated the metadata decoding part from the WriteInternal method

I have moved the metadata decoding code from WriteInternal to WriteInternalMetadata().
This method is able to decode the metadata even if it is not called from WriteInternal().

Comment 2

[Mihai Volmer [:mihaivolmer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf7f44ddcaf2

Comment 3

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8646122 [details] [diff] [review]
Separated the metadata decoding part from the WriteInternal method

Review of attachment 8646122 [details] [diff] [review]:
-----------------------------------------------------------------

It'd be good to think about how this is going to work once we have to read until gif_image_header to complete the metadata decode. That change is being made in bug 1191114.

::: image/decoders/nsGIFDecoder2.cpp
@@ +568,5 @@
> +uint32_t
> +nsGIFDecoder2::WriteInternalMetadata(const uint8_t* aBuffer, uint32_t aLength)
> +{ 
> +  MOZ_ASSERT(mGIFStruct.state == gif_type || mGIFStruct.state == gif_global_header,
> +    "This method should be called only for decoding the metadata part");

Nit: you should align the arguments to MOZ_ASSERT:

MOZ_ASSERT(veryLongCondition,
           "Message");

@@ +570,5 @@
> +{ 
> +  MOZ_ASSERT(mGIFStruct.state == gif_type || mGIFStruct.state == gif_global_header,
> +    "This method should be called only for decoding the metadata part");
> +
> +  uint8_t *q = (uint8_t*)aBuffer;

Nit: Mozilla style is |uint8_t* q|.

@@ +572,5 @@
> +    "This method should be called only for decoding the metadata part");
> +
> +  uint8_t *q = (uint8_t*)aBuffer;
> +
> +  // consumed keeps track of the number of bytes consumed from aBuffer

Nit: please write your comments as complete sentences, with capital letters at the beginning and periods at the end. If you don't do this, often the comments become unreadable when we make automatic changes to code formatting.

@@ +578,5 @@
> +  uint32_t consumed = 0;
> +  // loops ensures the for statement does not loop more than it should
> +  // This sould be edited if more cases are added in the metadata decode
> +  uint32_t loops = (mGIFStruct.state == gif_type) ? 2 :
> +    ((mGIFStruct.state == gif_global_header) ? 1 : 0);

Please don't use nested ternaries like this. Just use an if/else chain.

Also, could you explain why we need |loops| at all? As far as I can tell, it looks like |loops| will artificially hide bugs in this code.

@@ +593,5 @@
> +    case gif_type:
> +      if (!strncmp((char*)q, "GIF89a", 6)) {
> +        mGIFStruct.version = 89;
> +      }
> +      else if (!strncmp((char*)q, "GIF87a", 6)) {

Nit: Mozilla style is: |} else if (..) {|

@@ +602,5 @@
> +        return consumed;
> +      }
> +      GETN(7, gif_global_header);
> +      break;
> +      //return;

Delete this line.

@@ +634,5 @@
> +        if (aLength < size) {
> +          // Use 'hold' pattern to get the global colormap
> +          GETN(size, gif_global_colormap);
> +          break;
> +          //return consumed;

Delete this line.

@@ +763,5 @@
>      case gif_type:
> +      consumed = WriteInternalMetadata(q, len);
> +      consumed -= buf - q;
> +      buf += consumed;
> +      len -= consumed;

This looks unnecessarily complicated. Just pass in references to |buf| and |len| to WriteInternalMetadata. You shouldn't need |consumed| at all.

@@ +765,5 @@
> +      consumed -= buf - q;
> +      buf += consumed;
> +      len -= consumed;
> +      // Check if the metadata decoding has finished
> +      if (IsMetadataDecode() && GetImageMetadata().HasSize()) {

Just call |HasSize()| (the method on Decoder) instead of |GetImageMetadata().HasSize()|. Though it'd be better to just return a boolean from WriteInternalMetadata() indicating whether you're done, or something like that.

@@ +773,3 @@
>        break;
>  
>      case gif_global_header:

The code for this and gif_type should be identical, since they're both deferring to WriteInternalMetadata. Merge the two cases together. It should be like this:

case gif_type:
case gif_global_header:
  // ... code that calls WriteInternalMetadata

Comment 4

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 1) - Removed some obsolete lines from WriteInternal()

Comment 5

[Mihai Volmer [:mihaivolmer]]

Comment on attachment 8648275 [details] [diff] [review]
(Part 1) - Removed some obsolete lines from WriteInternal()

I have removed some obsolete code from WriteInternal().
The case where the method was called with nullptr and 0 as parameters is checked now in Decoder::Write().
The state gif_image_header_continue is no longer used.

Comment 6

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 2) - Added a separate method for global header parsing

This is just for reviewing purposes. It is not working when the hold pattern occurs.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8648275 [details] [diff] [review]
(Part 1) - Removed some obsolete lines from WriteInternal()

Review of attachment 8648275 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/decoders/nsGIFDecoder2.cpp
@@ -1041,5 @@
> -      }
> -      uint32_t realDepth = depth;
> -      while (mGIFStruct.tpixel >= (1 << realDepth) && (realDepth < 8)) {
> -        realDepth++;
> -      }

Why are you removing this stuff?

Comment 8

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8648275 [details] [diff] [review]
(Part 1) - Removed some obsolete lines from WriteInternal()

Review of attachment 8648275 [details] [diff] [review]:
-----------------------------------------------------------------

OK, we talked about this in person. It turns out the code I asked about is duplicated in the previous case statement.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8648277 [details] [diff] [review]
(Part 2) - Added a separate method for global header parsing

Review of attachment 8648277 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/decoders/nsGIFDecoder2.cpp
@@ +588,5 @@
> +{
> +  MOZ_ASSERT(mGIFStruct.state == gif_type || mGIFStruct.state == gif_global_header,
> +             "This method should be called only for decoding the metadata part");
> +
> +  uint8_t* q = (uint8_t*)aBuffer;

Likely this line is the source of the problem - you should pass both |q| and |buf| into this method, and not initialize |q| here.

@@ +598,5 @@
> +  // The variable loops ensures the for statement does
> +  // not loop more than it should. This sould be edited
> +  // if more cases are added in the metadata decode.
> +
> +  for (; aLength >= mGIFStruct.bytes_to_consume; q = (uint8_t*)aBuffer, mGIFStruct.bytes_in_hold = 0) {

Nit: avoid C-style casts in new code.

Maybe just make this method take a |uint8_t*| - no |const| - and do the const_cast<> at the callsite.

@@ +639,5 @@
> +      // screen_bgcolor is not used
> +      // mGIFStruct.screen_bgcolor = q[5];
> +      // q[6] = Pixel Aspect Ratio
> +      // Not used
> +      // float aspect = (float)((q[6] + 15) / 64.0);

Consider removing these lines in a separate patch. (You could do it in part 1, if you like, or add a new part.)

Comment 10

[Seth Fowler [:seth] [:s2h]]

We talked in person about the possibility of removing the gif_global_colormap/gif_image_colormap optimization. You should do this if this optimization is making the refactoring harder; we can file a followup to reintroduce it later, once the design has settled down.

The way this optimization works is that at the top of WriteInternal, if the current state is gif_global_colormap or gif_image_colormap, instead of writing the necessary data into the hold, we write it directly into the appropriate colormap buffer. That lets us avoid doing an extra copy in the gif_global_colormap/gif_image_colormap code.

To get rid of this optimization, in the code after the "Add what we have sofar to the block" comment, you should remove the gif_global_colormap and gif_image_colormap cases. You should then remove the special handling in gif_global_header and gif_image_header_continue, and just use the usual GETN pattern to read the colormaps. Instead, use memcpy in gif_global_colormap and gif_image_colormap to copy the data from |q| to the appropriate colormap buffer.

If you decide this is a good idea, you should do this in a separate patch. This is a complicated enough change on its own that it shouldn't be combined with anything else.

Comment 11

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 1) - Removed some obsolete lines from WriteInternal()

I have also removed some commented lines of code that were used to extract the screen_bgcolor and pixel aspect ratio attributes.

Comment 12

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 2) - Added a separate method for global header parsing

I have added one parameter to the ParseGlobalHeader() method: aHoldBuffer. When the hold pattern occurs, this will point to the holded buffer instead of the input buffer.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8649045 [details] [diff] [review]
(Part 2) - Added a separate method for global header parsing

Review of attachment 8649045 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good; we're almost there!

::: image/decoders/nsGIFDecoder2.cpp
@@ +582,5 @@
>      *--to = gfxPackedPixel(0xFF, from[0], from[1], from[2]);
>    }
>  }
> +uint32_t
> +nsGIFDecoder2::ParseGlobalHeader(const uint8_t* aBuffer, const uint8_t* aHoldBuffer, uint32_t aLength)

Nit: Remember there's a limit of 80 characters per line. It looks like you need to split this into multiple lines.

Also, add a blank line between this method and the preceding method.

@@ +589,5 @@
> +             mGIFStruct.state == gif_global_header ||
> +             mGIFStruct.state == gif_global_colormap,
> +             "This method should be called only for decoding the global header");
> +
> +  const uint8_t* q = (aHoldBuffer != nullptr ? aHoldBuffer : aBuffer);

const uint8_t* q = aHoldBuffer ? aHoldBuffer : aBuffer;

@@ +594,5 @@
> +
> +  // The variable consumed keeps track of the
> +  // number of bytes consumed from aBuffer.
> +  // The value of this variable will be returned.
> +  uint32_t consumed = 0;

Do we still need this? I'm pretty sure we don't if you pass aBuffer and aHoldBuffer as references. (|const uint8_t*&|) It'd be highly preferable to get rid of |consumed|.

@@ +654,5 @@
> +      ConvertColormap(mGIFStruct.global_colormap,
> +                      1 << mGIFStruct.global_colormap_depth);
> +      GETN(1, gif_image_start);
> +
> +      // Fall through

Just put a |break| after |GETN| here. It's better to avoid fall throughs when possible.

@@ +657,5 @@
> +
> +      // Fall through
> +
> +    default:
> +      // We have reached the end of the metadata decoding.

Assert what state you expect to be in here. Probably |MOZ_ASSERT(mGIFStruct.state == gif_image_start);|, right?

::: image/decoders/nsGIFDecoder2.h
@@ -40,5 @@
>    nsresult  BeginImageFrame(uint16_t aDepth);
>    void      EndImageFrame();
>    void      FlushImageData();
>    void      FlushImageData(uint32_t fromRow, uint32_t rows);
> -

Please don't get rid of this whitespace in this patch. It's better to separate formatting changes from refactoring.

Comment 14

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 2) - Added a separate method for global header parsing

I have modified the parameters to references. The ParseGlobalHeader() method now returns a bool regarding whether it did or did not finish the parsing.

Comment 15

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8649532 [details] [diff] [review]
(Part 2) - Added a separate method for global header parsing

Review of attachment 8649532 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the two issues below fixed.

The next step is a try job.

::: image/decoders/nsGIFDecoder2.cpp
@@ +592,5 @@
> +             mGIFStruct.state == gif_global_header ||
> +             mGIFStruct.state == gif_global_colormap,
> +             "This method should be called only for decoding the global header");
> +
> +  const uint8_t*& q = aHoldBuffer != nullptr ? aHoldBuffer : aBuffer;

Nit: please use |aHoldBuffer ? aHolderBuffer : aBuffer|. (See my previous review comments.)

@@ +641,5 @@
> +        GETN(0, gif_global_colormap);
> +        break;
> +      }
> +
> +      GETN(1, gif_image_start);

In this case the GIF has no global colormap. So shouldn't we be returning true here? Otherwise we'll hit the assertion in the default case...

Comment 16

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 2) - Added a separate method for global header parsing

Fixed a bug for gifs that do not have a global colormap.

Comment 17

[Mihai Volmer [:mihaivolmer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7435096467a7

Comment 18

[Seth Fowler [:seth] [:s2h]]

Those test results look good. If part 2 passes the tests added in bug 1196065, then I think it's ready to land.

Comment 19

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 2) - Added a separate method for global header parsing

Fixed a bug related to hold-pattern.

Comment 20

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8650175 [details] [diff] [review]
(Part 2) - Added a separate method for global header parsing

Review of attachment 8650175 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 21

[Seth Fowler [:seth] [:s2h]]

These changes were subsumed by a more general rewrite of the GIF decoder in bug 1204392.