From 279c448da35b982500f57b0b93c971ad6f5acb3e Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Tue, 11 Oct 2022 19:04:09 +0530 Subject: [PATCH] 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 | 16 +++-- app/file.go | 102 ++++++++++++++++------------ app/file_test.go | 2 +- app/slack.go | 8 +-- app/upload.go | 4 +- services/slackimport/slackimport.go | 12 ++-- tests/10000x1_expected_preview.jpeg | Bin 1076 -> 0 bytes tests/10000x1_expected_preview.png | Bin 0 -> 92 bytes tests/10000x1_expected_thumb.jpeg | Bin 628 -> 0 bytes tests/10000x1_expected_thumb.png | Bin 0 -> 76 bytes tests/1x10000_expected_preview.jpeg | Bin 3096 -> 0 bytes tests/1x10000_expected_preview.png | Bin 0 -> 121 bytes tests/1x10000_expected_thumb.jpeg | Bin 624 -> 0 bytes tests/1x10000_expected_thumb.png | Bin 0 -> 79 bytes 14 files changed, 83 insertions(+), 61 deletions(-) delete mode 100644 tests/10000x1_expected_preview.jpeg create mode 100644 tests/10000x1_expected_preview.png delete mode 100644 tests/10000x1_expected_thumb.jpeg create mode 100644 tests/10000x1_expected_thumb.png delete mode 100644 tests/1x10000_expected_preview.jpeg create mode 100644 tests/1x10000_expected_preview.png delete mode 100644 tests/1x10000_expected_thumb.jpeg create mode 100644 tests/1x10000_expected_thumb.png diff --git a/api4/file_test.go b/api4/file_test.go index 0184f972b7..85bd04ee21 100644 --- a/api4/file_test.go +++ b/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"}, - expectedImagePreviewNames: []string{"10000x1_expected_preview.jpeg"}, + expectedImageThumbnailNames: []string{"10000x1_expected_thumb.png"}, + 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"}, - expectedImagePreviewNames: []string{"1x10000_expected_preview.jpeg"}, + expectedImageThumbnailNames: []string{"1x10000_expected_thumb.png"}, + 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) - expectedPreviewPath := fmt.Sprintf("%s/%s_preview.jpg", expectedDir, name) + // We convert all other image types to jpeg, except pngs. + 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, diff --git a/app/file.go b/app/file.go index b0e68f4921..5f899aff70 100644 --- a/app/file.go +++ b/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.ThumbnailPath = pathPrefix + nameWithoutExtension + "_thumb.jpg" + info.PreviewPath = pathPrefix + nameWithoutExtension + "_preview." + getFileExtFromMimeType(info.MimeType) + 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.ThumbnailPath = t.pathPrefix() + nameWithoutExtension + "_thumb.jpg" + t.fileinfo.PreviewPath = t.pathPrefix() + nameWithoutExtension + "_preview." + getFileExtFromMimeType(t.fileinfo.MimeType) + 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.ThumbnailPath = pathPrefix + nameWithoutExtension + "_thumb.jpg" + info.PreviewPath = pathPrefix + nameWithoutExtension + "_preview." + getFileExtFromMimeType(info.MimeType) + 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) - }(img, thumbnailPathList[i]) + a.generateThumbnailImage(img, imgType, path) + }(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) - }(img, previewPathList[i]) + a.generatePreviewImage(img, imgType, path) + }(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)) - return + + thumb := imaging.GenerateThumbnail(img, imageThumbnailWidth, imageThumbnailHeight) + 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 { - mlog.Error("Unable to encode image as preview jpg", mlog.Err(err), mlog.String("path", previewPath)) - return + preview := imaging.GeneratePreview(img, imagePreviewWidth) + if imgType == "png" { + 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" +} diff --git a/app/file_test.go b/app/file_test.go index 599c4c6da1..09c03e640e 100644 --- a/app/file_test.go +++ b/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 diff --git a/app/slack.go b/app/slack.go index f7baeb53bc..b108b42980 100644 --- a/app/slack.go +++ b/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) { - img, release, err := prepareImage(a.ch.imgDecoder, bytes.NewReader(fileData)) + PrepareImage: func(fileData []byte) (image.Image, string, func(), error) { + 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 }, } diff --git a/app/upload.go b/app/upload.go index 15ccfba51a..3909f5e096 100644 --- a/app/upload.go +++ b/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.ThumbnailPath = filepath.Dir(info.Path) + "/" + nameWithoutExtension + "_thumb.jpg" + info.PreviewPath = filepath.Dir(info.Path) + "/" + nameWithoutExtension + "_preview." + getFileExtFromMimeType(info.MimeType) + info.ThumbnailPath = filepath.Dir(info.Path) + "/" + nameWithoutExtension + "_thumb." + getFileExtFromMimeType(info.MimeType) imgData, fileErr := a.ReadFile(uploadPath) if fileErr != nil { return nil, fileErr diff --git a/services/slackimport/slackimport.go b/services/slackimport/slackimport.go index 03863b202a..5b3b15df9f 100644 --- a/services/slackimport/slackimport.go +++ b/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) - GeneratePreviewImage func(image.Image, string) + GenerateThumbnailImage func(image.Image, string, 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.GeneratePreviewImage(img, fileInfo.PreviewPath) + si.actions.GenerateThumbnailImage(img, imgType, fileInfo.ThumbnailPath) + si.actions.GeneratePreviewImage(img, imgType, fileInfo.PreviewPath) } return fileInfo, nil diff --git a/tests/10000x1_expected_preview.jpeg b/tests/10000x1_expected_preview.jpeg deleted file mode 100644 index c547e21eefc9f60da050b4f7d8a8e0f352931f70..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 1076 zcmex=nP38T>!MAjrYM$lk!rsKme|$jB_n z`2PswA_fLVRz@&jfC5G)p!?X^IXJnv1sIqZnVFebm_e=us;mXdF|Y`-3Mm>ovIz$! zvMUve7&T5@$f4}C@t|nX#SbdRNkvVZTw>x9l2WQ_>Kd9_CZ=ZQ7M51dF0O9w9-dyo zA)#U65s^{JDXD4c8JStdC8cHM6_r)ZEv;?s9i3g1CQq3GGAU*RJ2VdF$b$$4{OPfBE|D`;VW$ z7#Wx$-T{&j4Gc)#w^HS&+zk7m&W?T T{9k=E7IiU<;^7$v|8D{SFo9Ub diff --git a/tests/10000x1_expected_preview.png b/tests/10000x1_expected_preview.png new file mode 100644 index 0000000000000000000000000000000000000000..bf2ca9cc218dc3f05531b743122fc2ae61df969e GIT binary patch literal 92 zcmeAS@N?(olHy`uVBq!ia0y~yU~d4j89A7MEaktG3U(zMMee&9;OZ5 o-`De~PPqLXD8;~#upTJ*|Nm`&Mi!n!HXA@fp00i_>zopr0H~W6vj6}9 literal 0 HcmV?d00001 diff --git a/tests/10000x1_expected_thumb.jpeg b/tests/10000x1_expected_thumb.jpeg deleted file mode 100644 index 90f2925324ff095484083398f9111f8bf51a8324..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 628 zcmex=nP38T>!MAjrYM$WXz|sKme|$jB_n z`2PswA_fLVRz@&jfC5G)p!?X^IXJnv1sIqZnVFebm_e=us;mXdF|Y`-3Mm>ovIz$! zvMUve7&T5@$f4}C@t|nX#SbdRNkvVZTw>x9l2WQ_>Kd9_CZ=ZQ7M51dF0O9w9-dyo zA)#U65s^{JDXD4c8JStdC8cHM6_r)ZEv;?s9i3g1CQq3GGAU*RJ2VdF$b$$4{OPfBE|D`;VW$ z7#Wx$-T{&j4Gc)#w^HS&+zk7m&W?T Q{9k=E7IiV;!vAjq0Em{u=Kufz diff --git a/tests/10000x1_expected_thumb.png b/tests/10000x1_expected_thumb.png new file mode 100644 index 0000000000000000000000000000000000000000..a354c410473a0afaaffa29debb380348e798873c GIT binary patch literal 76 zcmeAS@N?(olHy`uVBq!ia0vp^6+q0$!2~2{Y5(m3Qo^1tjv*Cul75~y;CjKxz{tSx Z|9|clMwS|xYbSux44$rjF6*2UngDQH6OsS` literal 0 HcmV?d00001 diff --git a/tests/1x10000_expected_preview.jpeg b/tests/1x10000_expected_preview.jpeg deleted file mode 100644 index e2511cb26fca3274a9d8617f7c2aab5eccc3adec..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 3096 zcmex=nP38T>!MAjqLEz`)4NsKme|$jB_n z`2PswA_fLVRz@&jfC5G)p!?X^IXJnv1sIqZnVFebm_e=us;mXdF|Y`-3Mm>ovIz$! zvMUve7&T5@$f4}C@t|nX#SbdRNkvVZTw>x9l2WQ_>Kd9_CZ=ZQ7M51dF0O9w9-dyo zA)#U65s^{JDXD4c8JStdC8cHM6_r)ZEv;?s9i3g1CQq3GGAU*RJ2VdF$b$$4{OPfBE|D`;VW$ z7#Wx$-T{&j4Gc)#w^HS&+zk7m&W?T o{9k=E7IiU<;?Xb|O#`E8U^ESkrh(BkFq#HN)4nP38T>!MAjrXx!obMPsKme|$jB_n z`2PswA_fLVRz@&jfC5G)p!?X^IXJnv1sIqZnVFebm_e=us;mXdF|Y`-3Mm>ovIz$! zvMUve7&T5@$f4}C@t|nX#SbdRNkvVZTw>x9l2WQ_>Kd9_CZ=ZQ7M51dF0O9w9-dyo zA)#U65s^{JDXD4c8JStdC8cHM6_r)ZEv;?s9i3g1CQq3GGAU*RJ2VdF$b$$4{OPfBE|D`;VW$ z7#Wx$-T{&j4Gc)#w^HS&+zk7m&W?T Q{9k=E7IiUT!~bsr09t{