From ba6e1b5dd5864c30560cabd6ef158ad95c6eff16 Mon Sep 17 00:00:00 2001 From: Alex Hart Date: Tue, 28 Jun 2022 16:37:36 -0300 Subject: [PATCH] Fix attachment deduplication issue with Stories. --- .../jobs/AttachmentCompressionJob.java | 19 +++++---- .../mediasend/v2/MediaSelectionRepository.kt | 8 ++-- .../securesms/mms/MediaStream.java | 9 ++++- .../mms/OutgoingSecureMediaMessage.java | 18 +++++++++ .../securesms/sms/MessageSender.java | 39 ++----------------- .../securesms/sms/OutgoingStoryMessage.kt | 8 ---- .../sms/UploadDependencyGraphTest.kt | 20 ++++++++++ 7 files changed, 64 insertions(+), 57 deletions(-) delete mode 100644 app/src/main/java/org/thoughtcrime/securesms/sms/OutgoingStoryMessage.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/AttachmentCompressionJob.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/AttachmentCompressionJob.java index 86a8608c95..dfe752b257 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/AttachmentCompressionJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/AttachmentCompressionJob.java @@ -49,6 +49,7 @@ import org.thoughtcrime.securesms.video.videoconverter.EncodingException; import java.io.ByteArrayInputStream; import java.io.File; import java.io.IOException; +import java.io.InputStream; import java.io.OutputStream; import java.util.Objects; import java.util.concurrent.TimeUnit; @@ -171,8 +172,9 @@ public final class AttachmentCompressionJob extends BaseJob { } } else if (constraints.canResize(attachment)) { Log.i(TAG, "Compressing image."); - MediaStream converted = compressImage(context, attachment, constraints); - attachmentDatabase.updateAttachmentData(attachment, converted, false); + try (MediaStream converted = compressImage(context, attachment, constraints)) { + attachmentDatabase.updateAttachmentData(attachment, converted, false); + } attachmentDatabase.markAttachmentAsTransformed(attachmentId); } else if (constraints.isSatisfied(context, attachment)) { Log.i(TAG, "Not compressing."); @@ -240,8 +242,9 @@ public final class AttachmentCompressionJob extends BaseJob { }, outputStream, cancelationSignal); } - MediaStream mediaStream = new MediaStream(ModernDecryptingPartInputStream.createFor(attachmentSecret, file, 0), MimeTypes.VIDEO_MP4, 0, 0); - attachmentDatabase.updateAttachmentData(attachment, mediaStream, true); + try (MediaStream mediaStream = new MediaStream(ModernDecryptingPartInputStream.createFor(attachmentSecret, file, 0), MimeTypes.VIDEO_MP4, 0, 0)) { + attachmentDatabase.updateAttachmentData(attachment, mediaStream, true); + } } finally { if (!file.delete()) { Log.w(TAG, "Failed to delete temp file"); @@ -259,15 +262,15 @@ public final class AttachmentCompressionJob extends BaseJob { if (transcoder.isTranscodeRequired()) { Log.i(TAG, "Compressing with android in-memory muxer"); - MediaStream mediaStream = transcoder.transcode(percent -> { + try (MediaStream mediaStream = transcoder.transcode(percent -> { notification.setProgress(100, percent); eventBus.postSticky(new PartProgressEvent(attachment, PartProgressEvent.Type.COMPRESSION, 100, percent)); - }, cancelationSignal); - - attachmentDatabase.updateAttachmentData(attachment, mediaStream, true); + }, cancelationSignal)) { + attachmentDatabase.updateAttachmentData(attachment, mediaStream, true); + } attachmentDatabase.markAttachmentAsTransformed(attachment.getAttachmentId()); diff --git a/app/src/main/java/org/thoughtcrime/securesms/mediasend/v2/MediaSelectionRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/mediasend/v2/MediaSelectionRepository.kt index 4e987f6d6b..3864358c16 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/mediasend/v2/MediaSelectionRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/mediasend/v2/MediaSelectionRepository.kt @@ -35,7 +35,6 @@ import org.thoughtcrime.securesms.mms.OutgoingMediaMessage import org.thoughtcrime.securesms.mms.OutgoingSecureMediaMessage import org.thoughtcrime.securesms.mms.SentMediaQuality import org.thoughtcrime.securesms.mms.Slide -import org.thoughtcrime.securesms.mms.VideoSlide import org.thoughtcrime.securesms.providers.BlobProvider import org.thoughtcrime.securesms.recipients.Recipient import org.thoughtcrime.securesms.scribbles.ImageEditorFragment @@ -299,7 +298,7 @@ class MediaSelectionRepository(context: Context) { OutgoingMediaMessage( recipient, body, - listOf(VideoSlide(context, it.uri, it.size, it.isVideoGif, it.width, it.height, it.caption.orElse(null), it.transformProperties.orElse(null)).asAttachment()), + listOf(MediaUploadRepository.asAttachment(context, it)), if (recipient.isDistributionList) distributionListStoryClipsSentTimestamps.getOrPut(it) { System.currentTimeMillis() } else System.currentTimeMillis(), -1, TimeUnit.SECONDS.toMillis(recipient.expiresInSeconds.toLong()), @@ -337,15 +336,14 @@ class MediaSelectionRepository(context: Context) { MessageSender.sendMediaBroadcast( context, nonStoryMessages, - preUploadResults, - Collections.emptyList() + preUploadResults ) } if (storyPreUploadMessages.isNotEmpty()) { Log.d(TAG, "Sending ${storyPreUploadMessages.size} preload messages to stories") storyPreUploadMessages.forEach { (preUploadResult, messages) -> - MessageSender.sendMediaBroadcast(context, messages, Collections.singleton(preUploadResult), Collections.emptyList()) + MessageSender.sendMediaBroadcast(context, messages, Collections.singleton(preUploadResult)) } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/mms/MediaStream.java b/app/src/main/java/org/thoughtcrime/securesms/mms/MediaStream.java index c736dd3a96..00db66b738 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/mms/MediaStream.java +++ b/app/src/main/java/org/thoughtcrime/securesms/mms/MediaStream.java @@ -16,9 +16,11 @@ */ package org.thoughtcrime.securesms.mms; +import java.io.Closeable; +import java.io.IOException; import java.io.InputStream; -public class MediaStream { +public class MediaStream implements Closeable { private final InputStream stream; private final String mimeType; private final int width; @@ -46,4 +48,9 @@ public class MediaStream { public int getHeight() { return height; } + + @Override + public void close() throws IOException { + stream.close(); + } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/mms/OutgoingSecureMediaMessage.java b/app/src/main/java/org/thoughtcrime/securesms/mms/OutgoingSecureMediaMessage.java index 5ecd639ada..d24968719b 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/mms/OutgoingSecureMediaMessage.java +++ b/app/src/main/java/org/thoughtcrime/securesms/mms/OutgoingSecureMediaMessage.java @@ -81,4 +81,22 @@ public class OutgoingSecureMediaMessage extends OutgoingMediaMessage { getMentions(), getGiftBadge()); } + + public @NonNull OutgoingSecureMediaMessage stripAttachments() { + return new OutgoingSecureMediaMessage(getRecipient(), + getBody(), + Collections.emptyList(), + getSentTimeMillis(), + getDistributionType(), + getExpiresIn(), + isViewOnce(), + getStoryType(), + getParentStoryId(), + isStoryReaction(), + getOutgoingQuote(), + Collections.emptyList(), + Collections.emptyList(), + getMentions(), + getGiftBadge()); + } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/sms/MessageSender.java b/app/src/main/java/org/thoughtcrime/securesms/sms/MessageSender.java index 52cb6ee8b9..9e0a858a13 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/sms/MessageSender.java +++ b/app/src/main/java/org/thoughtcrime/securesms/sms/MessageSender.java @@ -137,8 +137,7 @@ public class MessageSender { public static void sendStories(@NonNull final Context context, @NonNull final List messages, @Nullable final String metricId, - @Nullable final SmsDatabase.InsertListener insertListener - /** TODO [alex] -- Preupload set if preuploads were of valid length -- */) + @Nullable final SmsDatabase.InsertListener insertListener) { Log.i(TAG, "Sending story messages to " + messages.size() + " targets."); ThreadDatabase threadDatabase = SignalDatabase.threads(); @@ -149,10 +148,10 @@ public class MessageSender { try { database.beginTransaction(); - for (OutgoingMediaMessage message : messages) { + for (OutgoingSecureMediaMessage message : messages) { long allocatedThreadId = threadDatabase.getOrCreateValidThreadId(message.getRecipient(), -1L, message.getDistributionType()); Recipient recipient = message.getRecipient(); - long messageId = database.insertMessageOutbox(applyUniversalExpireTimerIfNecessary(context, recipient, message, allocatedThreadId), allocatedThreadId, false, insertListener); + long messageId = database.insertMessageOutbox(applyUniversalExpireTimerIfNecessary(context, recipient, message.stripAttachments(), allocatedThreadId), allocatedThreadId, false, insertListener); messageIds.add(messageId); threads.add(allocatedThreadId); @@ -320,8 +319,7 @@ public class MessageSender { public static void sendMediaBroadcast(@NonNull Context context, @NonNull List messages, - @NonNull Collection preUploadResults, - @NonNull List storyMessages) + @NonNull Collection preUploadResults) { Log.i(TAG, "Sending media broadcast to " + Stream.of(messages).map(m -> m.getRecipient().getId()).toList()); Preconditions.checkArgument(messages.size() > 0, "No messages!"); @@ -396,35 +394,6 @@ public class MessageSender { } } - for (final OutgoingStoryMessage storyMessage : storyMessages) { - OutgoingSecureMediaMessage message = storyMessage.getOutgoingSecureMediaMessage(); - - if (!message.getStoryType().isStory()) { - throw new AssertionError("Only story messages can be sent via this method."); - } - - long allocatedThreadId = threadDatabase.getOrCreateThreadIdFor(message.getRecipient(), message.getDistributionType()); - long messageId = mmsDatabase.insertMessageOutbox(storyMessage.getOutgoingSecureMediaMessage(), allocatedThreadId, false, null); - Optional preUploadAttachment = preUploadAttachments.stream() - .filter(a -> a.getAttachmentId().equals(storyMessage.getPreUploadResult().getAttachmentId())) - .findFirst(); - - if (!preUploadAttachment.isPresent()) { - Log.w(TAG, "Dropping story message without pre-upload attachment."); - mmsDatabase.markAsSentFailed(messageId); - } else { - AttachmentId attachmentCopyId = attachmentDatabase.insertAttachmentForPreUpload(preUploadAttachment.get()).getAttachmentId(); - attachmentDatabase.updateMessageId(Collections.singletonList(attachmentCopyId), messageId, true); - attachmentDatabase.updateAttachmentCaption(attachmentCopyId, storyMessage.getOutgoingSecureMediaMessage().getBody()); - messageIds.add(messageId); - messages.add(storyMessage.getOutgoingSecureMediaMessage()); - - Job copyJob = new AttachmentCopyJob(storyMessage.getPreUploadResult().getAttachmentId(), Collections.singletonList(attachmentCopyId)); - jobManager.add(copyJob, preUploadJobIds); - messageDependsOnIds.add(copyJob.getId()); - } - } - for (int i = 0; i < messageIds.size(); i++) { long messageId = messageIds.get(i); OutgoingSecureMediaMessage message = messages.get(i); diff --git a/app/src/main/java/org/thoughtcrime/securesms/sms/OutgoingStoryMessage.kt b/app/src/main/java/org/thoughtcrime/securesms/sms/OutgoingStoryMessage.kt deleted file mode 100644 index 59dbb8931e..0000000000 --- a/app/src/main/java/org/thoughtcrime/securesms/sms/OutgoingStoryMessage.kt +++ /dev/null @@ -1,8 +0,0 @@ -package org.thoughtcrime.securesms.sms - -import org.thoughtcrime.securesms.mms.OutgoingSecureMediaMessage - -class OutgoingStoryMessage( - val outgoingSecureMediaMessage: OutgoingSecureMediaMessage, - val preUploadResult: MessageSender.PreUploadResult -) diff --git a/app/src/test/java/org/thoughtcrime/securesms/sms/UploadDependencyGraphTest.kt b/app/src/test/java/org/thoughtcrime/securesms/sms/UploadDependencyGraphTest.kt index 1be638d26a..8056eb5a71 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/sms/UploadDependencyGraphTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/sms/UploadDependencyGraphTest.kt @@ -180,6 +180,26 @@ class UploadDependencyGraphTest { } } + @Test + fun `Given a list of attachments of the same uri with different transform props, when I consumeDeferredQueue, then I expect two chains`() { + val uriAttachments = (0..1).map { + UriAttachmentBuilder.build( + 1L, + contentType = MediaUtil.IMAGE_JPEG, + transformProperties = AttachmentDatabase.TransformProperties.forVideoTrim(it.toLong(), it.toLong() + 1) + ) + } + + val message = OutgoingMediaMessageBuilder.create(attachments = uriAttachments) + val testSubject = UploadDependencyGraph.create(listOf(message), jobManager) { getAttachmentForPreUpload(uniqueLong.getAndIncrement(), it) } + val result = testSubject.consumeDeferredQueue() + + assertEquals(2, result.size) + result.forEach { + assertValidJobChain(it, 0) + } + } + private fun assertValidJobChain(chain: JobManager.Chain, expectedCopyDestinationCount: Int) { val steps: List> = chain.jobListChain