847310 - Support WBMP on Firefox OS for MMS conformance

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

See OMA-TS-MMS-CONF-V1_3-20110913-A section 7, 7.1.1, and Appendix C.1.12

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: implement WBMP decoder

Implementation is referenced from bug 182621. I can wrap these modification with compile option if we only want support WBMP on Firefox OS.

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

this is needed to support MMS user stories. leo+

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: implement WBMP decoder, v2

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: test cases for WBMP decoder

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=f64f8fb60c82

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 722664 [details] [diff] [review]
implement WBMP decoder, v2

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

::: image/decoders/nsWBMPDecoder.cpp
@@ +75,5 @@
> +nsWBMPDecoder::~nsWBMPDecoder()
> +{
> +  if (mRow) {
> +    moz_free(mRow);
> +  }

no need to test for null here; moz_free handles null just fine.

@@ +95,5 @@
> +          // This is a type 0 WBMP.
> +          aCount--;
> +          mState = DecodingFixHeader;
> +        } else {
> +          // This is a new type or WBMP or a type 0 WBMP defined oddly (e.g. 0x80 0x00)

typo: "or" instead of "of"

@@ +163,5 @@
> +
> +          // If We're doing a size decode, we're done
> +          if (IsSizeDecode()) {
> +            mState = WbmpStateFinished;
> +            break;

That can be "return"

@@ +170,5 @@
> +          uint32_t imageLength;
> +          // Add the frame and signal
> +          nsresult rv = mImage.EnsureFrame(0, 0, 0, mWidth, mHeight,
> +                                  gfxASurface::ImageFormatRGB24,
> +                                  (uint8_t**)&mImageData, &imageLength);

please vertically align these arguments

@@ +192,5 @@
> +
> +          mState = DecodingImageData;
> +
> +        } else if (heightReadResult == IntParseFailed) {
> +          // Encoded width was bigger than a uint32_t.

s/width/height/

::: image/decoders/nsWBMPDecoder.h
@@ +16,5 @@
> +namespace image {
> +class RasterImage;
> +
> +/* Format description from http://www.wapforum.org/what/technical/SPEC-WAESpec-19990524.pdf */
> +

You should perhaps add a very brief comment here about what WBMP is, i.e. "WBMP is a monochrome graphics file format optimized for mobile computing devices"

@@ +60,5 @@
> +{
> +  uint8_t pixelValue = aPixelWhite ? 255 : 0;
> +
> +  *aDecoded++ = gfxPackedPixel(0xFF, pixelValue, pixelValue, pixelValue);
> +}

Can you put this in the cpp file instead of the header?

::: image/src/imgLoader.cpp
@@ +2049,5 @@
> +  // A well-defined type 0 WBMP file starts with an "0000 0000b" byte followed
> +  // by an "0xx0 0000b" byte (x = don't care).
> +  else if (aLength >= 2 &&
> +    ((unsigned char)aContents[0]==0x00 &&
> +    ((unsigned char)aContents[1] & 0x9F)==0x00) )

Can you add whitespace before and after ==, vertically align (( underneath aLength, and use static_cast<> here?

Comment 7

[Joe Drew (not getting mail)]

We're going to need a security review of this code.

Comment 8

[Joe Drew (not getting mail)]

I also filed bug 849391 in an attempt to get a security review, but I suspect that the form I used to file the request was for an entirely different thing.

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

can this bug land before finishing bug 849391? 
target to land if disregarding sec-review effort

Comment 10

[Joe Cheng [:jcheng] (please needinfo)]

i mean target to land this week if disregarding sec-review

Comment 11

[Joe Drew (not getting mail)]

No, this can't land before security review.

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: implement WBMP decoder, v3

Patch was updated according to comment 6, carry r+.

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: test cases for WBMP decoder, v2

test case cannot be executed on Fennec, skip android platform.

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

https://tbpl.mozilla.org/?tree=Try&rev=200815a0e155

Comment 15

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: implement WBMP decoder, v4

restrict width/height of a WBMP image to avoid overflow issue mentioned in bug 849812 (see https://bugzilla.mozilla.org/show_bug.cgi?id=849812#c9 ).

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8101722986a
https://hg.mozilla.org/integration/mozilla-inbound/rev/83720eb64f44

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we restrict use of this format to FirefoxOS install apps only? Or better still privileged apps? We don't want this format to be used by Web developers or app developers for their images.

Actually I'm wondering whether the MMS app could just decode these images itself in JS?

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e8101722986a
https://hg.mozilla.org/mozilla-central/rev/83720eb64f44

Comment 19

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> Can we restrict use of this format to FirefoxOS install apps only? Or better
> still privileged apps? We don't want this format to be used by Web
> developers or app developers for their images.
> 
> Actually I'm wondering whether the MMS app could just decode these images
> itself in JS?
WBMP is part of the WAP standard, so I think it is ok for Gecko to generally support this mobile web standard format.
Joe, have any suggestion?

Comment 20

[Joe Drew (not getting mail)]

I definitely want WBMP support restricted to Firefox OS.

Comment 21

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #20)
> I definitely want WBMP support restricted to Firefox OS.

Got it, I'll open another bug for the restriction.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See bug 847809 where we're restricting support for AMR to privileged apps only.

Comment 23

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> See bug 847809 where we're restricting support for AMR to privileged apps
> only.

hmm, suppost the browser app on FirefoxOS should be able to browse WAP site correctly, should we disable the WBMP support in browser app as well?

Comment 24

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

bug 852053 is filed as a follow-up bug for comment 17 and comment 20.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/c8405670d3bd
https://hg.mozilla.org/releases/mozilla-b2g18/rev/7bf4db4abef4

Comment 26

[Ryan VanderMeulen [:RyanVM]]

And a few bustage fixes because I suck at doing things right the first (or second) time.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/3b14247eeb6e
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5651355a1499
https://hg.mozilla.org/releases/mozilla-b2g18/rev/778da49486f0

Comment 27

[cajbir (:cajbir)]

This is possibly causing some media files not to play in B2G. See bug 857831 comment 11.

+  // A well-defined type 0 WBMP file starts with an "0000 0000b" byte followed
+  // by an "0xx0 0000b" byte (x = don't care).
+  else if (aLength >= 2 && (static_cast<unsigned char>(aContents[0]) == 0x00 &&
+                            (static_cast<unsigned char>(aContents[1]) & 0x9F) == 0x00))
+  {
+    aContentType.AssignLiteral(IMAGE_WBMP);
+  }

Isn't this resulting in any file beginning with 0x0000 to be a IMAGE_WBMP?

Comment 28

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@doublec, you're right, the WBMP header is too common to be used in mime type sniffer. I'm going to remove the WBMP content sniffer.