Closed Bug 1045468 Opened 5 years ago Closed 5 years ago

firefox doesn't support SVC NAL(NAL type 14)

Categories

(Core :: WebRTC: Audio/Video, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: ruil2, Assigned: ruil2)

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; WOW64; Trident/5.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; InfoPath.3; .NET4.0C; .NET4.0E)

Steps to reproduce:

interact with the product which creates SVC NAL


Actual results:

firefox can't decode the bitstream from the product with SVC NAL. no video display


Expected results:

firefox can dislay the video correclty.
Hardware: x86_64 → All
Summary: Don't support SVC NAL(NAL type 14) → firefox doesn't support SVC NAL(NAL type 14)
Attached patch add SVC NAL(NAL type 14)support (obsolete) — Splinter Review
modify code to support the prefix NAL for h.264
Comment on attachment 8463821 [details] [diff] [review]
add SVC NAL(NAL type 14)support

Review of attachment 8463821 [details] [diff] [review]:
-----------------------------------------------------------------

Setting Randell as reviewer.

::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc
@@ +300,5 @@
>            rtp_header->header.timestamp = timestamp - 10;
>            rtp_header->frameType = kVideoFrameKey;
>            break;
> +        case RtpFormatH264::kh264NALU_PREFIX:
> +          rtp_header->header.timestamp = timestamp - 10;

I believe the evil hack requires this timestamp changing to be different than the one above.   That is essentially what you have on the change below because of the fallthrough.  You get -30, -20, -10 there.  I'll defer to Randell on this and set him as reviewer.
Attachment #8463821 - Flags: review?(rjesup)
Comment on attachment 8463821 [details] [diff] [review]
add SVC NAL(NAL type 14)support

Review of attachment 8463821 [details] [diff] [review]:
-----------------------------------------------------------------

The same value is ok if (and only if) the PREFIX NAL will not be immediately adjacent to a PPS.  If you have SPS PPS PREFIX, it will fail.  (Or PREFIX PPS SPS, but that would be unexpected - I've always see SPS before PPS).

If it will never follow a PPS, -10 is fine. Otherwise choose a different value - and to avoid confusing it, the timestamps in that case must go in the "right" direction, so try -5.
Attachment #8463821 - Flags: review?(rjesup) → review+
Karina, please respond to Randell's comments above.  Let us know if you want to update this patch or go with what you have here and then I can push this for you tomorrow.
Assignee: nobody → ruil2
If you simply make it -5, it will handle all cases except PREFIX SPS or PREFIX PPS.  That'd be good by me, as I'm guessing you wouldn't see those sequences.
change prefix NAL timestamp
Hi Ethan: 
  I had updated the patch, what is the next step for this bug? thanks a lot!
Flags: needinfo?(ethanhugg)
Comment on attachment 8464396 [details] [diff] [review]
svc_nal_support_update.patch

Bringing forward r+ from jesup
Attachment #8464396 - Flags: review+
Flags: needinfo?(ethanhugg)
Attachment #8463821 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/641d9afb5bd2
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Component: Untriaged → WebRTC: Audio/Video
Product: Firefox → Core
Target Milestone: Firefox 34 → ---
Target Milestone: --- → mozilla34
Comment on attachment 8464396 [details] [diff] [review]
svc_nal_support_update.patch

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: PREFIX H.264 NALs won't be processed and ignored properly; forces rebase of Mode 0 support.

[Describe test coverage new/current, TBPL]: On m-c for several weeks; testing requires Cisco equipment that generates SVC-based streams.  Tested by Cisco

[Risks and why]: Very low; trivial patch to add some entries to case statements (basically).

[String/UUID change made/needed]: none
Attachment #8464396 - Flags: approval-mozilla-aurora?
Attachment #8464396 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.