diff --git a/src/calls.rs b/src/calls.rs index 2b8ce614b..e987cc4e4 100644 --- a/src/calls.rs +++ b/src/calls.rs @@ -220,13 +220,8 @@ impl Context { call.id = send_msg(self, chat_id, &mut call).await?; - // Store SDP offer in calls table - self.sql - .execute( - "INSERT OR REPLACE INTO calls (msg_id, offer_sdp) VALUES (?, ?)", - (call.id, &place_call_info), - ) - .await?; + // For outgoing calls, we don't store our own offer SDP in the database. + // It's only kept in memory for sending the message. let wait = RINGING_SECONDS; task::spawn(Context::emit_end_call_if_unaccepted( @@ -259,10 +254,10 @@ impl Context { chat.id.accept(self).await?; } - // Store SDP answer in calls table + // Store our answer SDP in calls table (replacing the offer from the other side) self.sql .execute( - "UPDATE calls SET answer_sdp=? WHERE msg_id=?", + "UPDATE calls SET sdp=? WHERE msg_id=?", (&accept_call_info, call_id), ) .await?; @@ -363,11 +358,11 @@ impl Context { from_id: ContactId, ) -> Result<()> { if mime_message.is_call() { - // Extract SDP from message headers and store in calls table + // Extract SDP offer from message headers and store in calls table for incoming calls if let Some(offer_sdp) = mime_message.get_header(HeaderDef::ChatWebrtcRoom) { self.sql .execute( - "INSERT OR IGNORE INTO calls (msg_id, offer_sdp) VALUES (?, ?)", + "INSERT OR IGNORE INTO calls (msg_id, sdp) VALUES (?, ?)", (call_id, offer_sdp), ) .await?; @@ -437,12 +432,13 @@ impl Context { return Ok(()); } - // Store SDP answer in calls table + // Store SDP answer in calls table for outgoing calls + // (for incoming calls, we've already replaced our offer with our answer in accept_incoming_call) if let Some(answer_sdp) = mime_message.get_header(HeaderDef::ChatWebrtcAccepted) { self.sql .execute( - "UPDATE calls SET answer_sdp=? WHERE msg_id=?", - (answer_sdp, call.msg.id), + "INSERT OR REPLACE INTO calls (msg_id, sdp) VALUES (?, ?)", + (call.msg.id, answer_sdp), ) .await?; } @@ -539,20 +535,26 @@ impl Context { // Load SDP from calls table. Returns empty strings if no record exists, // which can happen for old messages from before the migration or for // calls where SDPs have been cleaned up by housekeeping. - let (place_call_info, accept_call_info) = self + // For incoming calls, the SDP is the offer from the other side. + // For outgoing calls (after acceptance), the SDP is the answer from the other side. + let sdp = self .sql .query_row_optional( - "SELECT offer_sdp, answer_sdp FROM calls WHERE msg_id=?", + "SELECT sdp FROM calls WHERE msg_id=?", (call.id,), - |row| { - let offer: Option = row.get(0)?; - let answer: Option = row.get(1)?; - Ok((offer.unwrap_or_default(), answer.unwrap_or_default())) - }, + |row| row.get::<_, String>(0), ) .await? .unwrap_or_default(); + let (place_call_info, accept_call_info) = if call.from_id == ContactId::SELF { + // Outgoing call: the stored SDP (if any) is the answer from the other side + (String::new(), sdp) + } else { + // Incoming call: the stored SDP is the offer from the other side + (sdp, String::new()) + }; + Ok(Some(CallInfo { place_call_info, accept_call_info, diff --git a/src/calls/calls_tests.rs b/src/calls/calls_tests.rs index 23990026b..3fe41730f 100644 --- a/src/calls/calls_tests.rs +++ b/src/calls/calls_tests.rs @@ -682,21 +682,34 @@ async fn test_housekeeping_deletes_old_call_sdps() -> Result<()> { let alice = TestContext::new_alice().await; let bob = alice.create_chat_with_contact("", "bob@example.net").await; - // Place a call - let call_id = alice - .place_outgoing_call(bob.id, PLACE_INFO.to_string()) - .await?; + // Simulate receiving an incoming call from Bob + let received_call = receive_imf( + &alice, + b"From: bob@example.net\n\ + To: alice@example.org\n\ + Message-ID: \n\ + Chat-Version: 1.0\n\ + Chat-Content: call\n\ + Chat-Webrtc-Room: dGVzdC1zZHAtb2ZmZXI=\n\ + \n\ + Hello, this is a call\n", + false, + ) + .await? + .unwrap(); + + let call_id = received_call.msg_ids[0]; - // Verify SDP is stored in calls table + // Verify SDP is stored in calls table for incoming call let sdp_before: Option = alice .sql .query_row_optional( - "SELECT offer_sdp FROM calls WHERE msg_id=?", + "SELECT sdp FROM calls WHERE msg_id=?", (call_id,), |row| row.get(0), ) .await?; - assert_eq!(sdp_before, Some(PLACE_INFO.to_string())); + assert!(sdp_before.is_some()); // End the call alice.end_call(call_id).await?; @@ -709,12 +722,12 @@ async fn test_housekeeping_deletes_old_call_sdps() -> Result<()> { let sdp_after_end: Option = alice .sql .query_row_optional( - "SELECT offer_sdp FROM calls WHERE msg_id=?", + "SELECT sdp FROM calls WHERE msg_id=?", (call_id,), |row| row.get(0), ) .await?; - assert_eq!(sdp_after_end, Some(PLACE_INFO.to_string())); + assert!(sdp_after_end.is_some()); // Simulate passage of time - shift forward by 24 hours + 1 second SystemTime::shift(Duration::from_secs(86400 + 1)); @@ -726,7 +739,7 @@ async fn test_housekeeping_deletes_old_call_sdps() -> Result<()> { let sdp_after_housekeeping: Option = alice .sql .query_row_optional( - "SELECT offer_sdp FROM calls WHERE msg_id=?", + "SELECT sdp FROM calls WHERE msg_id=?", (call_id,), |row| row.get(0), ) diff --git a/src/mimefactory.rs b/src/mimefactory.rs index 61a0518dd..f61cef550 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -1694,10 +1694,11 @@ impl MimeFactory { .await?; if let Some(quoted_msg_id) = quoted_msg_id { + // For CallAccepted messages, retrieve the SDP (which is our answer) let answer_sdp = context .sql .query_row_optional( - "SELECT answer_sdp FROM calls WHERE msg_id=?", + "SELECT sdp FROM calls WHERE msg_id=?", (quoted_msg_id,), |row| row.get::<_, Option>(0), ) @@ -1750,24 +1751,15 @@ impl MimeFactory { } if msg.viewtype == Viewtype::Call { - // Get SDP offer from the message field (if being sent), calls table, or params (for old messages) + // Get SDP offer from the message field (if being sent) or params (for old messages). + // For outgoing calls, we don't store the offer in the database, only in memory. + // For incoming calls that are stored, we could query the database, but we typically + // only render outgoing call messages where we use the call_sdp_offer field. let offer_sdp = if let Some(ref offer) = msg.call_sdp_offer { Some(offer.clone()) - } else if !msg.id.is_unset() { - // Try to get from calls table if message is already stored - context - .sql - .query_row_optional( - "SELECT offer_sdp FROM calls WHERE msg_id=?", - (msg.id,), - |row| row.get::<_, Option>(0), - ) - .await? - .flatten() } else { - None - } - .or_else(|| msg.param.get(Param::WebrtcRoom).map(|s| s.to_string())); + msg.param.get(Param::WebrtcRoom).map(|s| s.to_string()) + }; if let Some(offer_sdp) = offer_sdp { headers.push(( diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index ff1151447..eb7dd46f2 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -1344,8 +1344,7 @@ CREATE INDEX gossip_timestamp_index ON gossip_timestamp (chat_id, fingerprint); sql.execute_migration( "CREATE TABLE calls( msg_id INTEGER PRIMARY KEY REFERENCES msgs(id) ON DELETE CASCADE, - offer_sdp TEXT, - answer_sdp TEXT, + sdp TEXT NOT NULL, ended_timestamp INTEGER ) STRICT;", migration_version,