amazon-chime-sdk-js icon indicating copy to clipboard operation
amazon-chime-sdk-js copied to clipboard

Conditional removal of H264 support is broken because H264 Camera sections are not found

Open jonathan-emed opened this issue 3 years ago • 4 comments

What happened and what did you expect to happen?

The function DefaultSDP.findActiveCameraSection incorrectly looks for sendrecv however the presence of that field is not guaranteed. In fact, it is assumed to be the default value if recvonly is not present.

https://github.com/aws/amazon-chime-sdk-js/blob/128fa920d85c3817d42406b900b37c71e57a98bf/src/sdp/DefaultSDP.ts#L78

See https://datatracker.ietf.org/doc/html/rfc4566

If none of the attributes "sendonly", "recvonly", "inactive",
and "sendrecv" is present, "sendrecv" SHOULD be assumed as the
default for sessions that are not of the conference type
"broadcast" or "H332" (see below).

How to Fix the Issue

if (sec.indexOf('recvonly') === -1) {
   hasCamera = true;
   break;
}

Have you reviewed our existing documentation?

Reproduction steps

Use the project in Safari

Amazon Chime SDK for JavaScript version

2.24.0

What browsers are you seeing the problem on?

Safari

Browser version

15.1

Meeting and Attendee ID Information.

No response

Browser console logs

NA

jonathan-emed avatar Jan 06 '22 16:01 jonathan-emed

@jonathan-emed Thanks for reporting. Could you add some details on how this affect your application use case?

ltrung avatar Jan 06 '22 18:01 ltrung

https://github.com/aws/amazon-chime-sdk-js/blob/128fa920d85c3817d42406b900b37c71e57a98bf/src/task/SetLocalDescriptionTask.ts#L36

Tries to remove H264 from the SDP under certain conditions and it doesn't work on some browsers which omit the sendrecv attribute because it isn't necessary or required as per the RFC... which is Safari because Safari does not produce sendrecv and the browser that workaround is trying to work for is... Safari.
https://github.com/aws/amazon-chime-sdk-js/blob/128fa920d85c3817d42406b900b37c71e57a98bf/src/sdp/DefaultSDP.ts#L544 Therefore the entire workaround for the Safari problem with H264 doesn't actually work on versions of Safari which does not produce the sendrecv tag.

jonathan-emed avatar Jan 06 '22 18:01 jonathan-emed

Hi @jonathan-emed, sorry for the delay, but appreciate seeing someone who enjoys looking at RFCs as much as I do :).

I'm onboard with your recommended change (i assume we would also want to check 'inactive' just in case the browser wants to throw that at the beginning of the function), but I am having difficulty reproducing. When I am using 15.2 MacOS safari (not sure there would be a difference between versions, I may be wrong though) I always see a=sendrecv. Additionally on the Munge SDP WebRTC sample it contains sendrecv (maybe its to do with https://github.com/aws/amazon-chime-sdk-js/blob/main/src/transceivercontroller/DefaultTransceiverController.ts#L340 )?

Anyways I don't mind making that modification, but am just curious about how to reproduce the issue. We had a good amount of testing to make sure that change fixed the iOS safari 15.1 bug and it didn't have issues.

hensmi-amazon avatar Jan 14 '22 01:01 hensmi-amazon

@hensmi-amazon I reproduced the issue on an earlier version of Safari in December. It could have been the iPad/iPhone or Desktop. Honestly, I don't remember. I only discovered it because it wasn't actually removing the H264 like expected. So, a version of Safari did exist in December, which required this change to our fork.

jonathan-emed avatar Jan 19 '22 21:01 jonathan-emed

Resolving as fixed

hensmi-amazon avatar Jan 23 '24 19:01 hensmi-amazon