Implement WiFi Display transport stream packetizer.

[chrome.displaySource] Implement transport stream packetizer.

The WiFi Display transport stream packetizer packetizes unit buffers first
to Packetized Elementary Stream (PES) packets (using a WiFi Display
elementary stream packetizer) and then to Transport Stream (TS) packets.
It will be used as a base class for a WiFi Display media packetizer.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/  <-- this CL
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer

BUG=242107
Committed: https://crrev.com/db33431413fe39fca0a34381092301d91ca66f40
Cr-Commit-Position: refs/heads/master@{#382706}

[e_hakkinen, 2016-03-14 10:26:00]

Description was changed from

==========
Implement WiFi Display transport stream packetizer.
==========

to

==========
Implement WiFi Display transport stream packetizer.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer
==========

[e_hakkinen, 2016-03-14 10:26:00]

eero.hakkinen@intel.com changed reviewers:
+ alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com

[e_hakkinen, 2016-03-14 11:18:49]

Description was changed from

==========
Implement WiFi Display transport stream packetizer.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer
==========

to

==========
[chrome.displaySource] Implement transport stream packetizer.

The WiFi Display transport stream packetizer packetizes unit buffers first
to Packetized Elementary Stream (PES) packets (using a WiFi Display
elementary stream packetizer) and then to Transport Stream (TS) packets.
It will be used as a base class for a WiFi Display media packetizer.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/  <-- this CL
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer

BUG=242107
==========

[e_hakkinen, 2016-03-14 14:26:16]

Patchset #1 (id:1) has been deleted

[e_hakkinen, 2016-03-14 14:46:53]

eero.hakkinen@intel.com changed reviewers:
+ asargent@chromium.org

[e_hakkinen, 2016-03-15 10:14:59]

Patchset #1 (id:20001) has been deleted

[asargent_no_longer_on_chrome, 2016-03-18 19:45:37]

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_info.h
(right):

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_info.h:51:
std::vector<WiFiDisplayElementaryStreamDescriptor> descriptors_;
suggest declaring this as DescriptorVector for consistency

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_media_packetizer_unittest.cc
(right):

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_media_packetizer_unittest.cc:86:
FakeTransportStreamPacketizer(Types... args)
nit: Variadic Template usage "should be rare":

http://chromium-cpp.appspot.com/


Do you really need this or can you rewrite with a fairly small number of
explicitly typed functions?

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.cc
(right):

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.cc:647:
for (auto&& stream_info : stream_infos) {
nit: rvalue references aren't allowed in this situation, see "Rvalue References"
section on

http://chromium-cpp.appspot.com/

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.h
(right):

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.h:19:
// WiFi Display transport stream packet represents an MPEG Transport Stream
consider shortening from "WiFi Display transport stream packet represents..." to
"This class represents..."

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.h:24:
enum { kSize = 188u };
readability could be improved with a comment or slightly more descriptive name -
is this the size of one packet? Consider naming it something like kPacketSize,
or include a comment to that effect.

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.h:28:
class PaddingPart {
nit: this class could use a brief comment describing what it's used for

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.h:79:
struct LPCM {
nit: include comment with brief description of what the LPCM acronym stands for

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.h:92:
bool EncodeElementaryStreamUnit(unsigned stream_index,
this function does enough interesting work that a brief comment is in order
about its overall purpose, possibly including things like any interesting
pre-conditions, whether it can be called multiple times, what the meaning of the
return value is, etc.

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.h:100:
void NormalizeUnitTimeStamps(base::TimeTicks* pts,
nit: these functions could use comments also

[e_hakkinen, 2016-03-22 08:02:04]

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_info.h
(right):

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_info.h:51:
std::vector<WiFiDisplayElementaryStreamDescriptor> descriptors_;
On 2016/03/18 19:45:36, Antony Sargent wrote:
> suggest declaring this as DescriptorVector for consistency

Done.

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_media_packetizer_unittest.cc
(right):

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_media_packetizer_unittest.cc:86:
FakeTransportStreamPacketizer(Types... args)
On 2016/03/18 19:45:36, Antony Sargent wrote:
> Do you really need this or can you rewrite with a fairly small number of
> explicitly typed functions?

Merely a leftover from a much earlier version where there used to be many ctors.
Now there is only one, so it is easy to use an explicitly typed one.

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.cc
(right):

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.cc:647:
for (auto&& stream_info : stream_infos) {
On 2016/03/18 19:45:36, Antony Sargent wrote:
> nit: rvalue references aren't allowed in this situation, see "Rvalue
References"
> section on
> 
> http://chromium-cpp.appspot.com/

Done.

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.h
(right):

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.h:19:
// WiFi Display transport stream packet represents an MPEG Transport Stream
On 2016/03/18 19:45:36, Antony Sargent wrote:
> consider shortening from "WiFi Display transport stream packet represents..."
to
> "This class represents..."

Done.

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.h:24:
enum { kSize = 188u };
On 2016/03/18 19:45:36, Antony Sargent wrote:
> readability could be improved with a comment or slightly more descriptive name
-
> is this the size of one packet?

Yes, it is.

> Consider naming it something like kPacketSize, or include a comment to that
> effect. 

Done.

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.h:28:
class PaddingPart {
On 2016/03/18 19:45:36, Antony Sargent wrote:
> nit: this class could use a brief comment describing what it's used for

Done.

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.h:79:
struct LPCM {
On 2016/03/18 19:45:36, Antony Sargent wrote:
> nit: include comment with brief description of what the LPCM acronym stands
for

Done.

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.h:92:
bool EncodeElementaryStreamUnit(unsigned stream_index,
On 2016/03/18 19:45:36, Antony Sargent wrote:
> this function does enough interesting work that a brief comment is in order
> about its overall purpose, possibly including things like any interesting
> pre-conditions, whether it can be called multiple times, what the meaning of
the
> return value is, etc.

Done.

https://codereview.chromium.org/1797953002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_transport_stream_packetizer.h:100:
void NormalizeUnitTimeStamps(base::TimeTicks* pts,
On 2016/03/18 19:45:36, Antony Sargent wrote:
> nit: these functions could use comments also

Done. I also moved NormalizeUnitTimeStamps to be protected as it should be only
used by WiFiDisplayTransportStreamPacketizer and its unit test.

[e_hakkinen, 2016-03-22 08:23:20]

Patchset #2 (id:60001) has been deleted

[asargent_no_longer_on_chrome, 2016-03-22 19:20:13]

lgtm

[e_hakkinen, 2016-03-22 21:07:37]

The CQ bit was checked by eero.hakkinen@intel.com

[commit-bot: I haz the power, 2016-03-22 21:07:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1797953002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1797953002/100001

[commit-bot: I haz the power, 2016-03-22 22:15:24]

Description was changed from

==========
[chrome.displaySource] Implement transport stream packetizer.

The WiFi Display transport stream packetizer packetizes unit buffers first
to Packetized Elementary Stream (PES) packets (using a WiFi Display
elementary stream packetizer) and then to Transport Stream (TS) packets.
It will be used as a base class for a WiFi Display media packetizer.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/  <-- this CL
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer

BUG=242107
==========

to

==========
[chrome.displaySource] Implement transport stream packetizer.

The WiFi Display transport stream packetizer packetizes unit buffers first
to Packetized Elementary Stream (PES) packets (using a WiFi Display
elementary stream packetizer) and then to Transport Stream (TS) packets.
It will be used as a base class for a WiFi Display media packetizer.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/  <-- this CL
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer

BUG=242107
==========

[commit-bot: I haz the power, 2016-03-22 22:15:26]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-03-22 22:16:49]

Description was changed from

==========
[chrome.displaySource] Implement transport stream packetizer.

The WiFi Display transport stream packetizer packetizes unit buffers first
to Packetized Elementary Stream (PES) packets (using a WiFi Display
elementary stream packetizer) and then to Transport Stream (TS) packets.
It will be used as a base class for a WiFi Display media packetizer.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/  <-- this CL
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer

BUG=242107
==========

to

==========
[chrome.displaySource] Implement transport stream packetizer.

The WiFi Display transport stream packetizer packetizes unit buffers first
to Packetized Elementary Stream (PES) packets (using a WiFi Display
elementary stream packetizer) and then to Transport Stream (TS) packets.
It will be used as a base class for a WiFi Display media packetizer.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/  <-- this CL
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer

BUG=242107

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

[commit-bot: I haz the power, 2016-03-22 22:16:50]

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