Remove now unused code in RtpHeaderExtensionMap

Remove now unused code in RtpHeaderExtensionMap
Remove functions to enumerate all extensions,
Remove concept of the inactive extension.
Decision if extension should be included into rtp header is done by rtp_sender
GetTotalLengthInBytes now calculates all extension, included or not.
That is used only for calculating how much space to reserve for fec.
Since extension might suddenly be included in the next packet (which still might belong to same fec group), it is safer to calculate all registered extension.

BUG=webrtc:5565, webrtc:1994
Committed: https://crrev.com/cc348338098133849063f2cab8023946a5cf42cb
Cr-Commit-Position: refs/heads/master@{#14763}

[danilchap, 2016-10-19 16:40:39]

Description was changed from

==========
Remove now unused code in RtpHeaderExtensionMap
Remove functions to enumerate all extensions,
Remove concept of the inactive extension.
Decision if extension should be included or not is done by rtp_sender
GetTotalLengthInBytes now calcultes all extension, included or not.
That is used only for calculating how much space to reserve for fec.
Since extension might be suddenly included in the next packet (which still
might belong to same fec group), it is safer to calculate all registered
extension.

BUG=webrtc:5565
==========

to

==========
Remove now unused code in RtpHeaderExtensionMap
Remove functions to enumerate all extensions,
Remove concept of the inactive extension.
Decision if extension should be included or not is done by rtp_sender
GetTotalLengthInBytes now calcultes all extension, included or not.
That is used only for calculating how much space to reserve for fec.
Since extension might be suddenly included in the next packet (which still
might belong to same fec group), it is safer to calculate all registered
extension.

BUG=webrtc:5565, webrtc:1994
==========

[danilchap, 2016-10-19 16:44:47]

danilchap@webrtc.org changed reviewers:
+ sprang@webrtc.org

[danilchap, 2016-10-19 16:44:48]

more code deletion (including few left-overs from previous one).
Further refactoring of RtpHeaderExtensionMap left to follow up CLs

[danilchap, 2016-10-19 16:46:02]

Description was changed from

==========
Remove now unused code in RtpHeaderExtensionMap
Remove functions to enumerate all extensions,
Remove concept of the inactive extension.
Decision if extension should be included or not is done by rtp_sender
GetTotalLengthInBytes now calcultes all extension, included or not.
That is used only for calculating how much space to reserve for fec.
Since extension might be suddenly included in the next packet (which still
might belong to same fec group), it is safer to calculate all registered
extension.

BUG=webrtc:5565, webrtc:1994
==========

to

==========
Remove now unused code in RtpHeaderExtensionMap
Remove functions to enumerate all extensions,
Remove concept of the inactive extension.
Decision if extension should be included into rtp header is done by rtp_sender
GetTotalLengthInBytes now calculates all extension, included or not.
That is used only for calculating how much space to reserve for fec.
Since extension might suddenly be included in the next packet (which still might
belong to same fec group), it is safer to calculate all registered extension.

BUG=webrtc:5565, webrtc:1994
==========

[sprang_webrtc, 2016-10-24 13:48:11]

https://codereview.chromium.org/2431253003/diff/1/webrtc/modules/rtp_rtcp/sou...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.chromium.org/2431253003/diff/1/webrtc/modules/rtp_rtcp/sou...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:250: if (video_header &&
video_header->rotation != kVideoRotation_0)
Just to be clear, how exactly is this rotation extension supposed to work...
The "rotation != kVideoRotation_0" makes me worried we can get into a situation,
where we toggle to 90 rotation and then back to 0, but 0 won't be sent so we're
stuck a 90? Or do we always interpret lack of rotation extension as implicit 0
rotation?

[danilchap, 2016-10-24 14:27:25]

https://codereview.chromium.org/2431253003/diff/1/webrtc/modules/rtp_rtcp/sou...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.chromium.org/2431253003/diff/1/webrtc/modules/rtp_rtcp/sou...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:250: if (video_header &&
video_header->rotation != kVideoRotation_0)
On 2016/10/24 13:48:10, språng wrote:
> Just to be clear, how exactly is this rotation extension supposed to work...
> The "rotation != kVideoRotation_0" makes me worried we can get into a
situation,
> where we toggle to 90 rotation and then back to 0, but 0 won't be sent so
we're
> stuck a 90? Or do we always interpret lack of rotation extension as implicit 0
> rotation?

No rotation extension means it is 0.
Before my refactorings this extension was also skipped when rotation was 0, even
if previous was 90:
bool video_rotation_active = false;
if (video_header && video_header->rotation != kVideoRotation_0) {
  video_rotation_active = rtp_sender_->ActivateCVORtpHeaderExtension();
}

But before, even if extension was skipped, place for it was reserved as soon as
there was non-zero rotation at least once. Now the space is used only when
extension is actually set.
Before place in fec was reserved only after non-zero value was observed. Now
[with this CL] it is always reserved. (sure only if it is registered and fec is
enabled).

[sprang_webrtc, 2016-10-25 07:47:01]

lgtm

https://codereview.webrtc.org/2431253003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2431253003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:250: if (video_header &&
video_header->rotation != kVideoRotation_0)
On 2016/10/24 14:27:25, danilchap wrote:
> On 2016/10/24 13:48:10, språng wrote:
> > Just to be clear, how exactly is this rotation extension supposed to work...
> > The "rotation != kVideoRotation_0" makes me worried we can get into a
> situation,
> > where we toggle to 90 rotation and then back to 0, but 0 won't be sent so
> we're
> > stuck a 90? Or do we always interpret lack of rotation extension as implicit
0
> > rotation?
> 
> No rotation extension means it is 0.
> Before my refactorings this extension was also skipped when rotation was 0,
even
> if previous was 90:
> bool video_rotation_active = false;
> if (video_header && video_header->rotation != kVideoRotation_0) {
>   video_rotation_active = rtp_sender_->ActivateCVORtpHeaderExtension();
> }
> 
> But before, even if extension was skipped, place for it was reserved as soon
as
> there was non-zero rotation at least once. Now the space is used only when
> extension is actually set.
> Before place in fec was reserved only after non-zero value was observed. Now
> [with this CL] it is always reserved. (sure only if it is registered and fec
is
> enabled). 

Acknowledged.

I interpreted the previous "rtp_sender_->ActivateCVORtpHeaderExtension()" call
to be sticky, so that the extension was first introduced when rotation != 0, but
was then kept for the rest of the call even when rotation returned to 0.

I'll trust you though. :)

[danilchap, 2016-10-25 09:41:10]

On 2016/10/25 07:47:01, språng wrote:
> lgtm
> 
>
https://codereview.webrtc.org/2431253003/diff/1/webrtc/modules/rtp_rtcp/sourc...
> File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):
> 
>
https://codereview.webrtc.org/2431253003/diff/1/webrtc/modules/rtp_rtcp/sourc...
> webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:250: if (video_header &&
> video_header->rotation != kVideoRotation_0)
> On 2016/10/24 14:27:25, danilchap wrote:
> > On 2016/10/24 13:48:10, språng wrote:
> > > Just to be clear, how exactly is this rotation extension supposed to
work...
> > > The "rotation != kVideoRotation_0" makes me worried we can get into a
> > situation,
> > > where we toggle to 90 rotation and then back to 0, but 0 won't be sent so
> > we're
> > > stuck a 90? Or do we always interpret lack of rotation extension as
implicit
> 0
> > > rotation?
> > 
> > No rotation extension means it is 0.
> > Before my refactorings this extension was also skipped when rotation was 0,
> even
> > if previous was 90:
> > bool video_rotation_active = false;
> > if (video_header && video_header->rotation != kVideoRotation_0) {
> >   video_rotation_active = rtp_sender_->ActivateCVORtpHeaderExtension();
> > }
> > 
> > But before, even if extension was skipped, place for it was reserved as soon
> as
> > there was non-zero rotation at least once. Now the space is used only when
> > extension is actually set.
> > Before place in fec was reserved only after non-zero value was observed. Now
> > [with this CL] it is always reserved. (sure only if it is registered and fec
> is
> > enabled). 
> 
> Acknowledged.
> 
> I interpreted the previous "rtp_sender_->ActivateCVORtpHeaderExtension()" call
> to be sticky, so that the extension was first introduced when rotation != 0,
but
> was then kept for the rest of the call even when rotation returned to 0.
> 

Actually, that is more correct than my description. Previous code didn't set
extension value
leaving it to default 0 (rotation_0) once activated. Now it is not set, which
breaks the standard (quoted in code).
But our receive side misinterpret the standard in same way: missing rotation
treated as rotation 0 instead of previous rotation.
I'll land this CL regardless (since I broke CVO in previous CL) and address CVO
issues in dedicated CLs.

[danilchap, 2016-10-25 10:11:11]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2016-10-25 10:11:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-25 10:12:30]

Description was changed from

==========
Remove now unused code in RtpHeaderExtensionMap
Remove functions to enumerate all extensions,
Remove concept of the inactive extension.
Decision if extension should be included into rtp header is done by rtp_sender
GetTotalLengthInBytes now calculates all extension, included or not.
That is used only for calculating how much space to reserve for fec.
Since extension might suddenly be included in the next packet (which still might
belong to same fec group), it is safer to calculate all registered extension.

BUG=webrtc:5565, webrtc:1994
==========

to

==========
Remove now unused code in RtpHeaderExtensionMap
Remove functions to enumerate all extensions,
Remove concept of the inactive extension.
Decision if extension should be included into rtp header is done by rtp_sender
GetTotalLengthInBytes now calculates all extension, included or not.
That is used only for calculating how much space to reserve for fec.
Since extension might suddenly be included in the next packet (which still might
belong to same fec group), it is safer to calculate all registered extension.

BUG=webrtc:5565, webrtc:1994
==========

[commit-bot: I haz the power, 2016-10-25 10:12:31]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-25 10:12:37]

Description was changed from

==========
Remove now unused code in RtpHeaderExtensionMap
Remove functions to enumerate all extensions,
Remove concept of the inactive extension.
Decision if extension should be included into rtp header is done by rtp_sender
GetTotalLengthInBytes now calculates all extension, included or not.
That is used only for calculating how much space to reserve for fec.
Since extension might suddenly be included in the next packet (which still might
belong to same fec group), it is safer to calculate all registered extension.

BUG=webrtc:5565, webrtc:1994
==========

to

==========
Remove now unused code in RtpHeaderExtensionMap
Remove functions to enumerate all extensions,
Remove concept of the inactive extension.
Decision if extension should be included into rtp header is done by rtp_sender
GetTotalLengthInBytes now calculates all extension, included or not.
That is used only for calculating how much space to reserve for fec.
Since extension might suddenly be included in the next packet (which still might
belong to same fec group), it is safer to calculate all registered extension.

BUG=webrtc:5565, webrtc:1994

Committed: https://crrev.com/cc348338098133849063f2cab8023946a5cf42cb
Cr-Commit-Position: refs/heads/master@{#14763}
==========

[commit-bot: I haz the power, 2016-10-25 10:12:38]

Patchset 1 (id:??) landed as
https://crrev.com/cc348338098133849063f2cab8023946a5cf42cb
Cr-Commit-Position: refs/heads/master@{#14763}