MM-36295: Do not convert thumbnails/previews to jpeg for png images (#21230)

We were applying a white background to transparent images
and converting them to jpegs. This was to make text be legible
behind a black preview background.

However, this has led to a poor user experience, as users rarely
download the full image but always click on previews. Therefore,
we need the previews to remain as pngs.

To fix this, we just re-encode them as pngs instead of jpgs.

```release-note
NONE
```

api4/file_test.go

@@ -397,8 +397,8 @@ func TestUploadFiles(t *testing.T) {
 		{
 			title:                       "Happy image thumbnail/preview 10",
 			names:                       []string{"10000x1.png"},
-			expectedImageThumbnailNames: []string{"10000x1_expected_thumb.jpeg"},
+			expectedImageThumbnailNames: []string{"10000x1_expected_thumb.png"},
-			expectedImagePreviewNames:   []string{"10000x1_expected_preview.jpeg"},
+			expectedImagePreviewNames:   []string{"10000x1_expected_preview.png"},
 			expectImage:                 true,
 			expectedImageWidths:         []int{10000},
 			expectedImageHeights:        []int{1},
@@ -409,8 +409,8 @@ func TestUploadFiles(t *testing.T) {
 		{
 			title:                       "Happy image thumbnail/preview 11",
 			names:                       []string{"1x10000.png"},
-			expectedImageThumbnailNames: []string{"1x10000_expected_thumb.jpeg"},
+			expectedImageThumbnailNames: []string{"1x10000_expected_thumb.png"},
-			expectedImagePreviewNames:   []string{"1x10000_expected_preview.jpeg"},
+			expectedImagePreviewNames:   []string{"1x10000_expected_preview.png"},
 			expectImage:                 true,
 			expectedImageWidths:         []int{1},
 			expectedImageHeights:        []int{10000},
@@ -678,8 +678,12 @@ func TestUploadFiles(t *testing.T) {
 						fmt.Sprintf("File %v saved to:%q, expected:%q", dbInfo.Name, dbInfo.Path, expectedPath))
 
 					if tc.expectImage {
-						expectedThumbnailPath := fmt.Sprintf("%s/%s_thumb.jpg", expectedDir, name)
+						// We convert all other image types to jpeg, except pngs.
-						expectedPreviewPath := fmt.Sprintf("%s/%s_preview.jpg", expectedDir, name)
+						if ext != ".png" {
+							ext = ".jpg"
+						}
+						expectedThumbnailPath := fmt.Sprintf("%s/%s_thumb%s", expectedDir, name, ext)
+						expectedPreviewPath := fmt.Sprintf("%s/%s_preview%s", expectedDir, name, ext)
 						assert.Equal(t, dbInfo.ThumbnailPath, expectedThumbnailPath,
 							fmt.Sprintf("Thumbnail for %v saved to:%q, expected:%q", dbInfo.Name, dbInfo.ThumbnailPath, expectedThumbnailPath))
 						assert.Equal(t, dbInfo.PreviewPath, expectedPreviewPath,

app/file.go

@@ -266,8 +266,8 @@ func (a *App) getInfoForFilename(post *model.Post, teamID, channelID, userID, ol
 
 	if info.IsImage() && !info.IsSvg() {
 		nameWithoutExtension := name[:strings.LastIndex(name, ".")]
-		info.PreviewPath = pathPrefix + nameWithoutExtension + "_preview.jpg"
+		info.PreviewPath = pathPrefix + nameWithoutExtension + "_preview." + getFileExtFromMimeType(info.MimeType)
-		info.ThumbnailPath = pathPrefix + nameWithoutExtension + "_thumb.jpg"
+		info.ThumbnailPath = pathPrefix + nameWithoutExtension + "_thumb." + getFileExtFromMimeType(info.MimeType)
 	}
 
 	return info
@@ -724,8 +724,8 @@ func (t *UploadFileTask) preprocessImage() *model.AppError {
 
 	t.fileinfo.HasPreviewImage = true
 	nameWithoutExtension := t.Name[:strings.LastIndex(t.Name, ".")]
-	t.fileinfo.PreviewPath = t.pathPrefix() + nameWithoutExtension + "_preview.jpg"
+	t.fileinfo.PreviewPath = t.pathPrefix() + nameWithoutExtension + "_preview." + getFileExtFromMimeType(t.fileinfo.MimeType)
-	t.fileinfo.ThumbnailPath = t.pathPrefix() + nameWithoutExtension + "_thumb.jpg"
+	t.fileinfo.ThumbnailPath = t.pathPrefix() + nameWithoutExtension + "_thumb." + getFileExtFromMimeType(t.fileinfo.MimeType)
 
 	// check the image orientation with goexif; consume the bytes we
 	// already have first, then keep Tee-ing from input.
@@ -770,20 +770,22 @@ func (t *UploadFileTask) postprocessImage(file io.Reader) {
 		defer release()
 	}
 
-	// Fill in the background of a potentially-transparent png file as white
-	if imgType == "png" {
-		imaging.FillImageTransparency(decoded, image.White)
-	}
-
 	decoded = imaging.MakeImageUpright(decoded, t.imageOrientation)
 	if decoded == nil {
 		return
 	}
 
-	writeJPEG := func(img image.Image, path string) {
+	writeImage := func(img image.Image, path string) {
 		r, w := io.Pipe()
 		go func() {
-			err := t.imgEncoder.EncodeJPEG(w, img, jpegEncQuality)
+			var err error
+			// It's okay to access imgType in a separate goroutine,
+			// because imgType is only written once and never written again.
+			if imgType == "png" {
+				err = t.imgEncoder.EncodePNG(w, img)
+			} else {
+				err = t.imgEncoder.EncodeJPEG(w, img, jpegEncQuality)
+			}
 			if err != nil {
 				mlog.Error("Unable to encode image as jpeg", mlog.String("path", path), mlog.Err(err))
 				w.CloseWithError(err)
@@ -804,12 +806,12 @@ func (t *UploadFileTask) postprocessImage(file io.Reader) {
 	// This is needed on mobile in case of animated GIFs.
 	go func() {
 		defer wg.Done()
-		writeJPEG(imaging.GenerateThumbnail(decoded, imageThumbnailWidth, imageThumbnailHeight), t.fileinfo.ThumbnailPath)
+		writeImage(imaging.GenerateThumbnail(decoded, imageThumbnailWidth, imageThumbnailHeight), t.fileinfo.ThumbnailPath)
 	}()
 
 	go func() {
 		defer wg.Done()
-		writeJPEG(imaging.GeneratePreview(decoded, imagePreviewWidth), t.fileinfo.PreviewPath)
+		writeImage(imaging.GeneratePreview(decoded, imagePreviewWidth), t.fileinfo.PreviewPath)
 	}()
 
 	go func() {
@@ -889,8 +891,8 @@ func (a *App) DoUploadFileExpectModification(c request.CTX, now time.Time, rawTe
 		}
 
 		nameWithoutExtension := filename[:strings.LastIndex(filename, ".")]
-		info.PreviewPath = pathPrefix + nameWithoutExtension + "_preview.jpg"
+		info.PreviewPath = pathPrefix + nameWithoutExtension + "_preview." + getFileExtFromMimeType(info.MimeType)
-		info.ThumbnailPath = pathPrefix + nameWithoutExtension + "_thumb.jpg"
+		info.ThumbnailPath = pathPrefix + nameWithoutExtension + "_thumb." + getFileExtFromMimeType(info.MimeType)
 	}
 
 	if pluginsEnvironment := a.GetPluginsEnvironment(); pluginsEnvironment != nil {
@@ -949,40 +951,33 @@ func (a *App) HandleImages(previewPathList []string, thumbnailPathList []string,
 	wg := new(sync.WaitGroup)
 
 	for i := range fileData {
-		img, release, err := prepareImage(a.ch.imgDecoder, bytes.NewReader(fileData[i]))
+		img, imgType, release, err := prepareImage(a.ch.imgDecoder, bytes.NewReader(fileData[i]))
 		if err != nil {
 			mlog.Debug("Failed to prepare image", mlog.Err(err))
 			continue
 		}
 		wg.Add(2)
-		go func(img image.Image, path string) {
+		go func(img image.Image, imgType, path string) {
 			defer wg.Done()
-			a.generateThumbnailImage(img, path)
+			a.generateThumbnailImage(img, imgType, path)
-		}(img, thumbnailPathList[i])
+		}(img, imgType, thumbnailPathList[i])
 
-		go func(img image.Image, path string) {
+		go func(img image.Image, imgType, path string) {
 			defer wg.Done()
-			a.generatePreviewImage(img, path)
+			a.generatePreviewImage(img, imgType, path)
-		}(img, previewPathList[i])
+		}(img, imgType, previewPathList[i])
 
 		wg.Wait()
 		release()
 	}
 }
 
-func prepareImage(imgDecoder *imaging.Decoder, imgData io.ReadSeeker) (img image.Image, release func(), err error) {
+func prepareImage(imgDecoder *imaging.Decoder, imgData io.ReadSeeker) (img image.Image, imgType string, release func(), err error) {
 	// Decode image bytes into Image object
-	var imgType string
 	img, imgType, release, err = imgDecoder.DecodeMemBounded(imgData)
 	if err != nil {
-		return nil, nil, fmt.Errorf("prepareImage: failed to decode image: %w", err)
+		return nil, "", nil, fmt.Errorf("prepareImage: failed to decode image: %w", err)
 	}
-
-	// Fill in the background of a potentially-transparent png file as white
-	if imgType == "png" {
-		imaging.FillImageTransparency(img, image.White)
-	}
-
 	imgData.Seek(0, io.SeekStart)
 
 	// Flip the image to be upright
@@ -992,14 +987,23 @@ func prepareImage(imgDecoder *imaging.Decoder, imgData io.ReadSeeker) (img image
 	}
 	img = imaging.MakeImageUpright(img, orientation)
 
-	return img, release, nil
+	return img, imgType, release, nil
 }
 
-func (a *App) generateThumbnailImage(img image.Image, thumbnailPath string) {
+func (a *App) generateThumbnailImage(img image.Image, imgType, thumbnailPath string) {
 	var buf bytes.Buffer
-	if err := a.ch.imgEncoder.EncodeJPEG(&buf, imaging.GenerateThumbnail(img, imageThumbnailWidth, imageThumbnailHeight), jpegEncQuality); err != nil {
+
-		mlog.Error("Unable to encode image as jpeg", mlog.String("path", thumbnailPath), mlog.Err(err))
+	thumb := imaging.GenerateThumbnail(img, imageThumbnailWidth, imageThumbnailHeight)
-		return
+	if imgType == "png" {
+		if err := a.ch.imgEncoder.EncodePNG(&buf, thumb); err != nil {
+			mlog.Error("Unable to encode image as png", mlog.String("path", thumbnailPath), mlog.Err(err))
+			return
+		}
+	} else {
+		if err := a.ch.imgEncoder.EncodeJPEG(&buf, thumb, jpegEncQuality); err != nil {
+			mlog.Error("Unable to encode image as jpeg", mlog.String("path", thumbnailPath), mlog.Err(err))
+			return
+		}
 	}
 
 	if _, err := a.WriteFile(&buf, thumbnailPath); err != nil {
@@ -1008,13 +1012,20 @@ func (a *App) generateThumbnailImage(img image.Image, thumbnailPath string) {
 	}
 }
 
-func (a *App) generatePreviewImage(img image.Image, previewPath string) {
+func (a *App) generatePreviewImage(img image.Image, imgType, previewPath string) {
 	var buf bytes.Buffer
-	preview := imaging.GeneratePreview(img, imagePreviewWidth)
 
-	if err := a.ch.imgEncoder.EncodeJPEG(&buf, preview, jpegEncQuality); err != nil {
+	preview := imaging.GeneratePreview(img, imagePreviewWidth)
-		mlog.Error("Unable to encode image as preview jpg", mlog.Err(err), mlog.String("path", previewPath))
+	if imgType == "png" {
-		return
+		if err := a.ch.imgEncoder.EncodePNG(&buf, preview); err != nil {
+			mlog.Error("Unable to encode image as preview png", mlog.Err(err), mlog.String("path", previewPath))
+			return
+		}
+	} else {
+		if err := a.ch.imgEncoder.EncodeJPEG(&buf, preview, jpegEncQuality); err != nil {
+			mlog.Error("Unable to encode image as preview jpg", mlog.Err(err), mlog.String("path", previewPath))
+			return
+		}
 	}
 
 	if _, err := a.WriteFile(&buf, previewPath); err != nil {
@@ -1033,7 +1044,7 @@ func (a *App) generateMiniPreview(fi *model.FileInfo) {
 			return
 		}
 		defer file.Close()
-		img, release, err := prepareImage(a.ch.imgDecoder, file)
+		img, _, release, err := prepareImage(a.ch.imgDecoder, file)
 		if err != nil {
 			mlog.Debug("generateMiniPreview: prepareImage failed", mlog.Err(err),
 				mlog.String("fileinfo_id", fi.Id), mlog.String("channel_id", fi.ChannelId),
@@ -1409,3 +1420,10 @@ func (a *App) getCloudFilesSizeLimit() (int64, *model.AppError) {
 
 	return int64(math.Ceil(float64(*limits.Files.TotalStorage) / 8)), nil
 }
+
+func getFileExtFromMimeType(mimeType string) string {
+	if mimeType == "image/png" {
+		return "png"
+	}
+	return "jpg"
+}

app/file_test.go

@@ -338,7 +338,7 @@ func TestGenerateThumbnailImage(t *testing.T) {
 		thumbnailPath := filepath.Join(dataPath, thumbnailName)
 
 		// when
-		th.App.generateThumbnailImage(img, thumbnailName)
+		th.App.generateThumbnailImage(img, "jpg", thumbnailName)
 		defer os.Remove(thumbnailPath)
 
 		// then

app/slack.go

@@ -40,12 +40,12 @@ func (a *App) SlackImport(c *request.Context, fileData multipart.File, fileSize
 		GeneratePreviewImage:   a.generatePreviewImage,
 		InvalidateAllCaches:    func() { a.ch.srv.InvalidateAllCaches() },
 		MaxPostSize:            func() int { return a.ch.srv.platform.MaxPostSize() },
-		PrepareImage: func(fileData []byte) (image.Image, func(), error) {
+		PrepareImage: func(fileData []byte) (image.Image, string, func(), error) {
-			img, release, err := prepareImage(a.ch.imgDecoder, bytes.NewReader(fileData))
+			img, imgType, release, err := prepareImage(a.ch.imgDecoder, bytes.NewReader(fileData))
 			if err != nil {
-				return nil, nil, err
+				return nil, "", nil, err
 			}
-			return img, release, err
+			return img, imgType, release, err
 		},
 	}
 

app/upload.go

@@ -298,8 +298,8 @@ func (a *App) UploadData(c *request.Context, us *model.UploadSession, rd io.Read
 		}
 
 		nameWithoutExtension := info.Name[:strings.LastIndex(info.Name, ".")]
-		info.PreviewPath = filepath.Dir(info.Path) + "/" + nameWithoutExtension + "_preview.jpg"
+		info.PreviewPath = filepath.Dir(info.Path) + "/" + nameWithoutExtension + "_preview." + getFileExtFromMimeType(info.MimeType)
-		info.ThumbnailPath = filepath.Dir(info.Path) + "/" + nameWithoutExtension + "_thumb.jpg"
+		info.ThumbnailPath = filepath.Dir(info.Path) + "/" + nameWithoutExtension + "_thumb." + getFileExtFromMimeType(info.MimeType)
 		imgData, fileErr := a.ReadFile(uploadPath)
 		if fileErr != nil {
 			return nil, fileErr

services/slackimport/slackimport.go

@@ -91,11 +91,11 @@ type Actions struct {
 	CreateGroupChannel     func(request.CTX, []string) (*model.Channel, *model.AppError)
 	CreateChannel          func(*model.Channel, bool) (*model.Channel, *model.AppError)
 	DoUploadFile           func(time.Time, string, string, string, string, []byte) (*model.FileInfo, *model.AppError)
-	GenerateThumbnailImage func(image.Image, string)
+	GenerateThumbnailImage func(image.Image, string, string)
-	GeneratePreviewImage   func(image.Image, string)
+	GeneratePreviewImage   func(image.Image, string, string)
 	InvalidateAllCaches    func()
 	MaxPostSize            func() int
-	PrepareImage           func(fileData []byte) (image.Image, func(), error)
+	PrepareImage           func(fileData []byte) (image.Image, string, func(), error)
 }
 
 // SlackImporter is a service that allows to import slack dumps into mattermost
@@ -793,13 +793,13 @@ func (si *SlackImporter) oldImportFile(timestamp time.Time, file io.Reader, team
 	}
 
 	if fileInfo.IsImage() && !fileInfo.IsSvg() {
-		img, release, err := si.actions.PrepareImage(data)
+		img, imgType, release, err := si.actions.PrepareImage(data)
 		if err != nil {
 			return nil, err
 		}
 		defer release()
-		si.actions.GenerateThumbnailImage(img, fileInfo.ThumbnailPath)
+		si.actions.GenerateThumbnailImage(img, imgType, fileInfo.ThumbnailPath)
-		si.actions.GeneratePreviewImage(img, fileInfo.PreviewPath)
+		si.actions.GeneratePreviewImage(img, imgType, fileInfo.PreviewPath)
 	}
 
 	return fileInfo, nil