The default bug view has changed. See this FAQ.

Support WBMP on Firefox OS for MMS conformance

RESOLVED FIXED in Firefox 22

Status

()

Core
ImageLib
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: schien, Assigned: schien)

Tracking

unspecified
mozilla22
Other
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:leo+, firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

(Whiteboard: [3/11~3/15])

Attachments

(2 attachments, 4 obsolete attachments)

See OMA-TS-MMS-CONF-V1_3-20110913-A section 7, 7.1.1, and Appendix C.1.12
Created attachment 721608 [details] [diff] [review]
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.
Attachment #721608 - Flags: review?(joe)
this is needed to support MMS user stories. leo+
blocking-b2g: --- → leo+
Created attachment 722664 [details] [diff] [review]
implement WBMP decoder, v2
Attachment #721608 - Attachment is obsolete: true
Attachment #721608 - Flags: review?(joe)
Attachment #722664 - Flags: review?(joe)
Created attachment 722667 [details] [diff] [review]
test cases for WBMP decoder
Attachment #722667 - Flags: review?(joe)
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?
Attachment #722664 - Flags: review?(joe) → review+
Attachment #722667 - Flags: review?(joe) → review+
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.
Flags: sec-review?
can this bug land before finishing bug 849391? 
target to land if disregarding sec-review effort
Whiteboard: [3/11~3/15]
i mean target to land this week if disregarding sec-review
Depends on: 849812
No, this can't land before security review.
Created attachment 723864 [details] [diff] [review]
implement WBMP decoder, v3

Patch was updated according to comment 6, carry r+.
Attachment #722664 - Attachment is obsolete: true
Attachment #723864 - Flags: review+
Created attachment 723865 [details] [diff] [review]
test cases for WBMP decoder, v2

test case cannot be executed on Fennec, skip android platform.
Attachment #722667 - Attachment is obsolete: true
Attachment #723865 - Flags: review+
https://tbpl.mozilla.org/?tree=Try&rev=200815a0e155
Created attachment 725247 [details] [diff] [review]
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 ).
Attachment #723864 - Attachment is obsolete: true
Attachment #725247 - Flags: review+
Attachment #725247 - Flags: feedback?(cdiehl)
Attachment #725247 - Flags: feedback?(cdiehl) → feedback+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8101722986a
https://hg.mozilla.org/integration/mozilla-inbound/rev/83720eb64f44
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(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?
Flags: needinfo?(joe)
I definitely want WBMP support restricted to Firefox OS.
Flags: needinfo?(joe)
(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?
Depends on: 852053
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
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → wontfix
status-firefox22: --- → fixed
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
Flags: sec-review?

Comment 27

4 years ago
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.

Updated

4 years ago
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.