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

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

this is needed to support MMS user stories. leo+

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

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?

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

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.

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

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

No, this can't land before security review.

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

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

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 ).

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

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?

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

(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?

I definitely want WBMP support restricted to Firefox OS.

(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.

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

(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?

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

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

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

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?

@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.