Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1752243002:
Modifying video_frame.cc to copy all metadata values, instead of only END_OF_STREAM, and updating u… (Closed)
DescriptionModifying VideoFrame to copy all metadata values to wrapped frame in
WrapVideoFrame.
VideoFrame only copied the VideoFrameMetadata::END_OF_STREAM value when creating
a wrapping frame. This change adds a MergeMetadataFrom utility function to
VideoFrameMetadata and uses it to copy all the values from the original frame's
metadata into the wrapped frame.
R=watk@chromium.org
BUG=591138
TEST=updated unit tests
Committed: https://crrev.com/24ff05db02b42e939d2f10fdbaae75a83db60120
Cr-Commit-Position: refs/heads/master@{#379166}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Addressing Comments #Patch Set 3 : Addressing comments #
Total comments: 4
Patch Set 4 : Addressing last comment #
Messages
Total messages: 23 (11 generated)
The CQ bit was checked by tguilbert@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1752243002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1752243002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tguilbert@chromium.org changed reviewers: + dalecurtis@chromium.org, watk@chromium.org
Nice. One thing that's not obvious is that the CL description gets turned into the git commit message when the bot commits this change, so you'll want to clean it up. There's some guidance here https://www.chromium.org/developers/contributing-code#TOC-Change-List-Descrip... There's also the command "git cl description" if you'd prefer to edit it in an editor. Also we typically refer to the class name rather than the file, so s/video_frame.cc/VideoFrame. https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame.cc#n... media/base/video_frame.cc:520: // Copy all metada to the wrapped frame s/metada/metadata Full stop/period at the end of the comment. (https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G...) https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame.cc#n... media/base/video_frame.cc:523: wrapping_frame->metadata()->MergeInternalValuesFrom(temp_metadata_values); WDYT about adding a VideoFrameMetadata::MergeMetadataFrom? https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame_unit... File media/base/video_frame_unittest.cc (right): https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame_unit... media/base/video_frame_unittest.cc:258: // Verify metada was copied to the wrapped frame s/metada/metadata Full stop. https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame_unit... media/base/video_frame_unittest.cc:264: if (frame_has_duration) { No need to handle !frame_has_duration because that's a test failure anyway. You could change l.260 to ASSERT_TRUE to abort the test if the metadata isn't there: ASSERT_TRUE(frame->metadata()->GetTimeDelta( media::VideoFrameMetadata::FRAME_DURATION, &frame_duration); https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame_unit... media/base/video_frame_unittest.cc:267: // Verify the metadata copy was a deep copy Full stop.
Description was changed from ========== Modifying video_frame.cc to copy all metadata values, instead of only END_OF_STREAM, and updating unit tests BUG=591138 ========== to ========== Modifying VideoFrame to copy all metadata values to wrapped frame in WrapVideoFrame. VideoFrame only copied the VideoFrameMetadata::END_OF_STREAM value when creating a wrapping frame. This change adds a MergeMetadataFrom utility function to VideoFrameMetadata and uses it to copy all the values from the original frame's metadata R=watk@chromium.org BUG=591138 TEST=updated unit tests ==========
Description was changed from ========== Modifying VideoFrame to copy all metadata values to wrapped frame in WrapVideoFrame. VideoFrame only copied the VideoFrameMetadata::END_OF_STREAM value when creating a wrapping frame. This change adds a MergeMetadataFrom utility function to VideoFrameMetadata and uses it to copy all the values from the original frame's metadata R=watk@chromium.org BUG=591138 TEST=updated unit tests ========== to ========== Modifying VideoFrame to copy all metadata values to wrapped frame in WrapVideoFrame. VideoFrame only copied the VideoFrameMetadata::END_OF_STREAM value when creating a wrapping frame. This change adds a MergeMetadataFrom utility function to VideoFrameMetadata and uses it to copy all the values from the original frame's metadata in the WrapVideoFrame method R=watk@chromium.org BUG=591138 TEST=updated unit tests ==========
Description was changed from ========== Modifying VideoFrame to copy all metadata values to wrapped frame in WrapVideoFrame. VideoFrame only copied the VideoFrameMetadata::END_OF_STREAM value when creating a wrapping frame. This change adds a MergeMetadataFrom utility function to VideoFrameMetadata and uses it to copy all the values from the original frame's metadata in the WrapVideoFrame method R=watk@chromium.org BUG=591138 TEST=updated unit tests ========== to ========== Modifying VideoFrame to copy all metadata values to wrapped frame in WrapVideoFrame. VideoFrame only copied the VideoFrameMetadata::END_OF_STREAM value when creating a wrapping frame. This change adds a MergeMetadataFrom utility function to VideoFrameMetadata and uses it to copy all the values from the original frame's metadata in the WrapVideoFrame method. R=watk@chromium.org BUG=591138 TEST=updated unit tests ==========
Description was changed from ========== Modifying VideoFrame to copy all metadata values to wrapped frame in WrapVideoFrame. VideoFrame only copied the VideoFrameMetadata::END_OF_STREAM value when creating a wrapping frame. This change adds a MergeMetadataFrom utility function to VideoFrameMetadata and uses it to copy all the values from the original frame's metadata in the WrapVideoFrame method. R=watk@chromium.org BUG=591138 TEST=updated unit tests ========== to ========== Modifying VideoFrame to copy all metadata values to wrapped frame in WrapVideoFrame. VideoFrame only copied the VideoFrameMetadata::END_OF_STREAM value when creating a wrapping frame. This change adds a MergeMetadataFrom utility function to VideoFrameMetadata and uses it to copy all the values from the original frame's metadata R=watk@chromium.org BUG=591138 TEST=updated unit tests ==========
Description was changed from ========== Modifying VideoFrame to copy all metadata values to wrapped frame in WrapVideoFrame. VideoFrame only copied the VideoFrameMetadata::END_OF_STREAM value when creating a wrapping frame. This change adds a MergeMetadataFrom utility function to VideoFrameMetadata and uses it to copy all the values from the original frame's metadata R=watk@chromium.org BUG=591138 TEST=updated unit tests ========== to ========== Modifying VideoFrame to copy all metadata values to wrapped frame in WrapVideoFrame. VideoFrame only copied the VideoFrameMetadata::END_OF_STREAM value when creating a wrapping frame. This change adds a MergeMetadataFrom utility function to VideoFrameMetadata and uses it to copy all the values from the original frame's metadata into the wrapped frame. R=watk@chromium.org BUG=591138 TEST=updated unit tests ==========
https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame.cc#n... media/base/video_frame.cc:520: // Copy all metada to the wrapped frame On 2016/03/02 21:25:16, watk wrote: > s/metada/metadata > > Full stop/period at the end of the comment. > (https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G...) Done. https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame.cc#n... media/base/video_frame.cc:523: wrapping_frame->metadata()->MergeInternalValuesFrom(temp_metadata_values); On 2016/03/02 21:25:16, watk wrote: > WDYT about adding a VideoFrameMetadata::MergeMetadataFrom? I am fine with the idea. Should I pro-actively modify files that used the serialization/de-serialization elsewhere in the codebase to use this new method? https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame_unit... File media/base/video_frame_unittest.cc (right): https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame_unit... media/base/video_frame_unittest.cc:258: // Verify metada was copied to the wrapped frame On 2016/03/02 21:25:16, watk wrote: > s/metada/metadata > > Full stop. Done. https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame_unit... media/base/video_frame_unittest.cc:264: if (frame_has_duration) { On 2016/03/02 21:25:16, watk wrote: > No need to handle !frame_has_duration because that's a test failure anyway. You > could change l.260 to ASSERT_TRUE to abort the test if the metadata isn't there: > > ASSERT_TRUE(frame->metadata()->GetTimeDelta( > media::VideoFrameMetadata::FRAME_DURATION, &frame_duration); Good to know, thank you for the tip. https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame_unit... media/base/video_frame_unittest.cc:267: // Verify the metadata copy was a deep copy On 2016/03/02 21:25:16, watk wrote: > Full stop. Done.
defer to watk@ for review! https://codereview.chromium.org/1752243002/diff/40001/media/base/video_frame_... File media/base/video_frame_metadata.cc (right): https://codereview.chromium.org/1752243002/diff/40001/media/base/video_frame_... media/base/video_frame_metadata.cc:148: void VideoFrameMetadata::MergeMetadataFrom( After this lands there are a few places you can cleanup too: https://code.google.com/p/chromium/codesearch#search/&q=base::DictionaryValue... (not all of those are valid, but some are)
https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1752243002/diff/1/media/base/video_frame.cc#n... media/base/video_frame.cc:523: wrapping_frame->metadata()->MergeInternalValuesFrom(temp_metadata_values); On 2016/03/03 21:42:04, ThomasGuilbert wrote: > On 2016/03/02 21:25:16, watk wrote: > > WDYT about adding a VideoFrameMetadata::MergeMetadataFrom? > > I am fine with the idea. Should I pro-actively modify files that used the > serialization/de-serialization elsewhere in the codebase to use this new method? Nice, I think a follow up CL to clean those up makes sense. https://codereview.chromium.org/1752243002/diff/40001/media/base/video_frame_... File media/base/video_frame_metadata.cc (right): https://codereview.chromium.org/1752243002/diff/40001/media/base/video_frame_... media/base/video_frame_metadata.cc:152: MergeInternalValuesFrom(temp_metadata_values); I think you can simplify this to: dictionary_.MergeDictionary(&metadata_source->dictionary_);
https://codereview.chromium.org/1752243002/diff/40001/media/base/video_frame_... File media/base/video_frame_metadata.cc (right): https://codereview.chromium.org/1752243002/diff/40001/media/base/video_frame_... media/base/video_frame_metadata.cc:148: void VideoFrameMetadata::MergeMetadataFrom( On 2016/03/03 21:44:51, DaleCurtis wrote: > After this lands there are a few places you can cleanup too: > > https://code.google.com/p/chromium/codesearch#search/&q=base::DictionaryValue... > > (not all of those are valid, but some are) Will do! https://codereview.chromium.org/1752243002/diff/40001/media/base/video_frame_... media/base/video_frame_metadata.cc:152: MergeInternalValuesFrom(temp_metadata_values); On 2016/03/03 22:54:33, watk wrote: > I think you can simplify this to: > > dictionary_.MergeDictionary(&metadata_source->dictionary_); Done.
lgtm
The CQ bit was checked by tguilbert@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1752243002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1752243002/60001
Message was sent while issue was closed.
Description was changed from ========== Modifying VideoFrame to copy all metadata values to wrapped frame in WrapVideoFrame. VideoFrame only copied the VideoFrameMetadata::END_OF_STREAM value when creating a wrapping frame. This change adds a MergeMetadataFrom utility function to VideoFrameMetadata and uses it to copy all the values from the original frame's metadata into the wrapped frame. R=watk@chromium.org BUG=591138 TEST=updated unit tests ========== to ========== Modifying VideoFrame to copy all metadata values to wrapped frame in WrapVideoFrame. VideoFrame only copied the VideoFrameMetadata::END_OF_STREAM value when creating a wrapping frame. This change adds a MergeMetadataFrom utility function to VideoFrameMetadata and uses it to copy all the values from the original frame's metadata into the wrapped frame. R=watk@chromium.org BUG=591138 TEST=updated unit tests ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Modifying VideoFrame to copy all metadata values to wrapped frame in WrapVideoFrame. VideoFrame only copied the VideoFrameMetadata::END_OF_STREAM value when creating a wrapping frame. This change adds a MergeMetadataFrom utility function to VideoFrameMetadata and uses it to copy all the values from the original frame's metadata into the wrapped frame. R=watk@chromium.org BUG=591138 TEST=updated unit tests ========== to ========== Modifying VideoFrame to copy all metadata values to wrapped frame in WrapVideoFrame. VideoFrame only copied the VideoFrameMetadata::END_OF_STREAM value when creating a wrapping frame. This change adds a MergeMetadataFrom utility function to VideoFrameMetadata and uses it to copy all the values from the original frame's metadata into the wrapped frame. R=watk@chromium.org BUG=591138 TEST=updated unit tests Committed: https://crrev.com/24ff05db02b42e939d2f10fdbaae75a83db60120 Cr-Commit-Position: refs/heads/master@{#379166} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/24ff05db02b42e939d2f10fdbaae75a83db60120 Cr-Commit-Position: refs/heads/master@{#379166} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
