Re: [9fans] a go workaround for the broken jpg(1)...

Quoth ori-C8/[email protected]:
> Quoth ori-C8/[email protected]:
> > Quoth ori-C8/[email protected]:
> > > Quoth [email protected]:
> > > > Hello 9fans,
> > > > For me, jpg(1) can't display most jpeg images I have.  So I made a go
> > > > program that convert a jpeg image from stdin to a png image to stdout.
> > > > Assume that Plan 9's png is not broken ;)
> > > > 
> > > > To test, go to https://thuychi.vn/anh.html
> > > > Of 50 images, Plan 9's jpg could only decode 3.  The go library can
> > > > decode all of them.
> > > > 
> > > > I hope that there will be go image/plan9 and a draw library that works
> > > > on Plan 9 too...  Replace the jpg(1) programs with the Go version!
> > > > 
> > > > bind /bin/jpgtopng.rc /bin/jpg, I think at least abaco don't need to
> > > > be modified.
> > > > 
> > > 
> > > Great, I think you just reminded me of an old bug in jpg that I'd
> > > been looking into (and then got distracted from). It looks like when
> > > we decode jpg images, we mishandle progresive jpgs that have scans
> > > that don't use interleaving, and have subsampled components. These
> > > aren't too common, but you'll run across them in the wild.
> > > 
> > > Normally, we'll end up with the luminance channel coming through,
> > > but the chroma channels completely broken.
> > > Now that I'm looking at it again, I think this has something to do
> > > with how we walk the scans; data either comes in MCU or scan order,
> > > and we may be mangling this.
> > > 
> > > this may not be so hard to fix.
> > > 
> > 
> > Alright, I think I found/fixed up the patch; there were two
> > issues that we had; first, when decoding the DC components, for
> > scans where Ns=1, we need to handle them in raster order, not
> > MCU order[1].  The change to progressivedc() handles the Nf==1
> > case to deal with the different scan order.
> > 
> > The second case was that we don't need to have all frame
> > components present in the scan, and we needed to use Csj (the
> > component selector to pick the appropriate mage components from
> > the scan header.
> > 
> > [1] https://www.w3.org/Graphics/JPEG/itu-t81.pdf, A.2.2
> > [2] https://www.w3.org/Graphics/JPEG/itu-t81.pdf, B.2.3
> > 
> 
> Whoops, had an out of bounds write, and now that I'm paging it
> in, I think I want to refactor the change and detangle a bit.

Ok -- this should be both working, and much easier to understand.
Instead of tangling both Nf=1 and Nf!=1 cases, it splits them;
the two cases naturally stand alone.

diff 07c4ebae5b344643d4a00b4532d216ef7291e42a uncommitted
--- a/sys/src/cmd/jpg/readjpg.c
+++ b/sys/src/cmd/jpg/readjpg.c
@@ -457,7 +457,7 @@
 		return 0;
 	}
 	if(Bread(h->fd, tmp, 2) != 2)
-    Readerr:
+Readerr:
 		jpgerror(h, readerr);
 	n = int2(tmp, 0);
 	if(n < 2)
@@ -574,7 +574,7 @@
 		t->shift[v] = 8-cnt;
 		t->value[v] = t->val[t->valptr[i]+(code-t->mincode[i])];
 
-    continueBytes:
+continueBytes:
 		v++;
 	}
 
@@ -899,16 +899,15 @@
 void
 progressivedc(Header *h, int comp, int Ah, int Al)
 {
-	int Ns, z, ri, mcu,  nmcu;
+	int Ns, z, ri, mcu, nmcu;
 	int block, t, diff, qt, *dc, bn;
+	int x, y, H, V, nhor, nver, q;
 	Huffman *dcht;
 	uchar *ss;
-	int i, Td[3], DC[3], blockno[3];
+	int i, j, Td[4], DC[4], scancomp[4], blockno[4];
 
 	ss= h->ss;
 	Ns = ss[0];
-	if(Ns!=1 && Ns!=h->Nf)
-		jpgerror(h, "ReadJPG: can't handle progressive with Ns!=1 and Nf!=Ns in DC scan");
 
 	/* initialize data structures */
 	h->cnt = 0;
@@ -916,47 +915,81 @@
 	h->peek = -1;
 
 	for(i=0; i<Ns; i++){
-		/*
-		 * JPEG requires scan components to be in same order as in frame,
-		 * so if both have 3 we know scan is Y Cb Cr and there's no need to
-		 * reorder
-		 */
 		nibbles(ss[2+2*i], &Td[i], &z);	/* z is ignored */
 		DC[i] = 0;
+		for(j=0; j<h->Nf; j++)
+			if(h->comp[j].C == ss[1+2*i]){
+				scancomp[i] = j;
+				break;
+			}
+		if(j == h->Nf)
+			jpgerror(h, "ReadJPG: bad component selector in scan");
 	}
 
 	ri = h->ri;
 
-	nmcu = h->nacross*h->ndown;
-	memset(blockno, 0, sizeof blockno);
-	for(mcu=0; mcu<nmcu; ){
-		for(i=0; i<Ns; i++){
-			if(Ns != 1) comp = i;
-
-			dcht = &h->dcht[Td[i]];
-			qt = h->qt[h->comp[comp].Tq][0];
-			dc = h->dccoeff[comp];
-			bn = blockno[i];
-
-			for(block=0; block<h->nblock[comp]; block++){
+	if(Ns == 1){
+		/* ITU-T.81 A.2.2: scan in raster order if Ns = 1*/
+		comp = scancomp[0];
+		H = h->comp[comp].H;
+		V = h->comp[comp].V;
+		q = 8*h->Hmax/H;
+		nhor = (h->X+q-1)/q;
+		q = 8*h->Vmax/V;
+		nver = (h->Y+q-1)/q;
+		dcht = &h->dcht[Td[0]];
+		qt = h->qt[h->comp[comp].Tq][0];
+		dc = h->dccoeff[comp];
+		mcu = 0;
+		for(y=0; y<nver; y++){
+			for(x=0; x<nhor; x++){
+				bn = (x/H + h->nacross*(y/V))*H*V + H*(y%V) + x%H;
 				if(Ah == 0){
 					t = decode(h, dcht);
 					diff = receive(h, t);
-					DC[i] += diff;
-					dc[bn] = qt*DC[i]<<Al;
+					DC[0] += diff;
+					dc[bn] = qt*DC[0]<<Al;
 				}else
 					dc[bn] |= qt*receivebit(h)<<Al;
-				bn++;
+				/* process restart marker, if present */
+				mcu++;
+				if(ri>0 && mcu%ri==0){
+					restart(h, mcu);
+					DC[0] = 0;
+				}
 			}
-			blockno[i] = bn;
 		}
-
-		/* process restart marker, if present */
-		mcu++;
-		if(ri>0 && mcu<nmcu && mcu%ri==0){
-			restart(h, mcu);
-			for(i=0; i<Ns; i++)
-				DC[i] = 0;
+	}else{
+		/* ITU-T.81 A.2.3: scan in MCU order otherwise */
+		nmcu = h->nacross*h->ndown;
+		memset(blockno, 0, sizeof blockno);
+		for(mcu=0; mcu<nmcu; ){
+			for(i=0; i<Ns; i++){
+				comp = scancomp[i];
+				dcht = &h->dcht[Td[i]];
+				qt = h->qt[h->comp[comp].Tq][0];
+				dc = h->dccoeff[comp];
+				bn = blockno[i];
+				for(block=0; block<h->nblock[comp]; block++){
+					if(Ah == 0){
+						t = decode(h, dcht);
+						diff = receive(h, t);
+						DC[i] += diff;
+						dc[bn] = qt*DC[i]<<Al;
+					}else
+						dc[bn] |= qt*receivebit(h)<<Al;
+					bn++;
+				}
+				blockno[i] = bn;
+			}
+	
+			/* process restart marker, if present */
+			mcu++;
+			if(ri>0 && mcu<nmcu && mcu%ri==0){
+				restart(h, mcu);
+				for(i=0; i<Ns; i++)
+					DC[i] = 0;
+			}
 		}
 	}
 }
@@ -1003,7 +1036,7 @@
 	mcu = 0;
 	for(y=0; y<nver; y++){
 		for(x=0; x<nhor; x++){
-			/* Figure G-3  */
+			/* Figure G-3 */
 			if(eobrun > 0){
 				--eobrun;
 				continue;
@@ -1104,7 +1137,7 @@
 		for(x=0; x<nhor; x++){
 			/* Figure G-7 */
 
-			/*  arrange blockno to be in same sequence as original scan calculation. */
+			/* arrange blockno to be in same sequence as original scan calculation. */
 			tmcu = x/H + (nacross/H)*(y/V);
 			blockno = tmcu*H*V + H*(y%V) + x%H;
 			acc = ac[blockno];
@@ -1237,7 +1270,9 @@
 
 static
 void
-colormapall1(Header *h, int colorspace, Rawimage *image, int data0[8*8], int data1[8*8], int data2[8*8], int mcu, int nacross)
+colormapall1(Header *h, int colorspace, Rawimage *image,
+	int data0[8*8], int data1[8*8], int data2[8*8],
+	int mcu, int nacross)
 {
 	uchar *rpic, *gpic, *bpic, *rp, *gp, *bp;
 	int *p0, *p1, *p2;
@@ -1290,7 +1325,10 @@
 
 static
 void
-colormap(Header *h, int colorspace, Rawimage *image, int *data0[8*8], int *data1[8*8], int *data2[8*8], int mcu, int nacross, int Hmax, int Vmax,  int *H, int *V)
+colormap(Header *h, int colorspace, Rawimage *image,
+	int *data0[8*8], int *data1[8*8], int *data2[8*8],
+	int mcu, int nacross, int Hmax, int Vmax,
+	int *H, int *V)
 {
 	uchar *rpic, *gpic, *bpic;
 	int x, y, dx, dy, minx, miny;