1328543 - [Mortar] colors are wrong while changing, resizing, scrolling pages on pdf

Status: RESOLVED WORKSFORME

(Firefox :: PDF Viewer, defect, P2)

Description

[Ya-Chieh Wu (inactive)]

Right now, we have problems with rendering right colors as we saw some pdf with weird colors. As far as I studied, we mismatch red pixel to blue pixel and blue to red. We need to fix it.

Comment 3

[Evelyn Hung]

I also found sometimes, the wrong pixel rendering happened when you resize window.

Comment 4

[Ya-Chieh Wu (inactive)]

(In reply to Ya-Chieh Wu from comment #0)
> Right now, we have problems with rendering right colors as we saw some pdf
> with weird colors. As far as I studied, we mismatch red pixel to blue pixel
> and blue to red. We need to fix it.

After investigating, the root cause of this problem is that we didn't perform the color conversion right on mortar's image buffer. Now, we did color conversion per 2d operation. However, several 2d operations might use one same image buffer so every time we executed a 2d operation, we exchange the place of red and blue pixels. That's why some times the colors are right, some times the colors are wrong.

Comment 5

[Ya-Chieh Wu (inactive)]

Attachment: Fix the wrong colors on pdf while scrolling, changing, resizing pages

This patch should fix the issue that was mentioned on Comment 4.

Comment 6

[Ya-Chieh Wu (inactive)]

Attachment: fix the wrong colors on pdf while scrolling, changing pages and resizing

This patch is basically the same as Comment 5. This should fix the issue that was mentioned on Comment 4.

Comment 7

[Ya-Chieh Wu (inactive)]

Comment on attachment 8827333 [details] [diff] [review]
fix the wrong colors on pdf while scrolling, changing pages and resizing

Peter, thank you for your reviewing.

This patch is trying to avoid we convert GBRA format of |imageData| to RGBA format more than once. As the fact that one |imageData| is using in different 2doperations, we should only convert the color form of |imageData| once.

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 8827333 [details] [diff] [review]
fix the wrong colors on pdf while scrolling, changing pages and resizing

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

::: browser/extensions/mortar/host/common/ppapi-runtime.jsm
@@ +968,5 @@
>      this.operations = [];
>    }
>  
>    flush(callback) {
> +    let RGBABuffers = [];

I think I'd prefer to store whether we've converted the colors on the ImageData object itself. For example with a boolean member that we check in getDOMImageData before calling _colorConvert. I'm not entirely sure if it's possible, but it seems like we should then also be able to throw an error in beginDrawing if we've already converted the colors.

Comment 9

[Ya-Chieh Wu (inactive)]

(In reply to Peter Van der Beken [:peterv] from comment #8)
> ::: browser/extensions/mortar/host/common/ppapi-runtime.jsm
> @@ +968,5 @@
> > this.operations = [];
> > }
> > 
> > flush(callback) {
> > + let RGBABuffers = [];
> 
> I think I'd prefer to store whether we've converted the colors on the ImageData object itself. For example with a boolean member that we check in getDOMImageData before calling _colorConvert. 

Thanks, Peter. I was thinking the same thing at first but I choose another way to implement because I was thinking that if it will be better that we implement in this way so it is clear that we convert the color bits in one flush operation or the boolean check we scatter in getDOMImageData, PPB_Graphics2D_PaintImageData, and PPB_Graphics2D_ReplaceContents. I will provide another version and you can make the call. 

> > I'm not entirely sure if it's possible, but it seems like we should then also be able to throw an error in beginDrawing if we've already converted the colors. 

The operation here is different from |Graphics2DPaintOperation|. The code surround by |beginDrawing| and |endDrawing| are drawing by ourself[1] so we might not need to do anything here? no?


[1] https://dxr.mozilla.org/mozilla-central/source/browser/extensions/mortar/host/common/ppapi-runtime.jsm#2227

Comment 10

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug1328543-wip.patch

Peter, 
In this patch, I save a boolean in the image class to indicate if the color has been converted. However, how pdfium operates the buffer is not that intuitive so I would prefer to use our previous' patch. Let me explain it to you below. 

PPAPI calls: 
1.PaintImageData - imageData_x(BGRA format)
2.PaintImageData - imageData_x(BGRA format)     
3.PaintImageData - imageData_y(BGRA format)
4.flush // we apply BGRA to RGBA here.
5.PaintImageData - imageData_x(*BGRA format*)   // Still use *imageData_x*, but the data is back to BGRA format not RGBA format. So we shouldn't do any conversion here.
6.flush // we apply BGRA to RGBA here.

As you can see, after the first flush, we assume that the imageData_x has been converted but it turns out that pdfium somehow replace the whole buffer so the buffer state is back to unconverted. That's why I would prefer to check if the color is been converted when we do flush instead of saving a boolean in the imageData.  

how do you think?

Comment 11

[Ya-Chieh Wu (inactive)]

Note: I just had a discussion with astley yesterday and he gave a valuable input that we probably can find out if we can configure pdfium to draw the imageData into RGBA format so that we can avoide to do the color conversion which can improve the scolling perfomance or in general when users change pages. 

Anyway, this bug still needs to be reolved to take care of the case of "BGRA format".

Comment 12

[Peter Van der Beken [:peterv]]

(In reply to Ya-Chieh Wu from comment #11)
> Note: I just had a discussion with astley yesterday and he gave a valuable
> input that we probably can find out if we can configure pdfium to draw the
> imageData into RGBA format so that we can avoide to do the color conversion
> which can improve the scolling perfomance or in general when users change
> pages. 

I looked into that when I first wrote this code, the PDF plugin specifically asks for BGRA (see https://chromium.googlesource.com/chromium/src.git/+/ec747d5400f2930853e1f90c8365916393803606/pdf/out_of_process_instance.cc#647).

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 8863968 [details] [diff] [review]
0001-Bug1328543-wip.patch

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

::: browser/extensions/mortar/host/common/ppapi-runtime.jsm
@@ +2814,5 @@
>       */
>      PPB_Graphics2D_ReplaceContents: function(json) {
>        let graphics = PP_Resource.lookup(json.graphics_2d);
>        let imageData = PP_Resource.lookup(json.image_data);
> +      imageData.colorConverted = false;

I think I'd prefer if you did this inside the Graphics2DPaintOperation constructor when x == y == 0 and dirtyRect == []. I think that would work? It doesn't make sense to set colorConverted to false if we're not overwriting the whole imageData (from ReplaceContents), because we'd leave pieces in the wrong format.

Comment 14

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug-1328543-fix-the-wrong-colors-on-pdf-while-scroll.patch

Peter, 
I think that we shouldn't convert the origin buffer at all because it is not right that we don't convert it back and keep pdfium using it afterward. I think what we should do is do not contaminate the origin buffer and save a copy of RGBA buffer. I know this is not that perfect but by doing so, we guarantee the pure of the origin buffer and save the efforts to convert it back.

How do you think?

Comment 15

[Ya-Chieh Wu (inactive)]

has no plan for this.
close it now.