WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#35218 closed enhancement (fixed)

Parse the creation date out of uploaded videos

Reported by: stevegrunwell Owned by: mikeschroder
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

When images are uploaded into the media library, wp_read_image_metadata() will read the images' EXIF data to determine the date the image was taken, which is returned in "created_timestamp". Other media types (audio and video) do not provide this information, but it's still useful metadata (especially on media-heavy sites).

This information is available through the getID3 library, however where it lives varies based on the type of media uploaded. I propose we add a "created_timestamp" key to wp_read_video_metadata() (and ideally wp_read_audio_metadata() as well) which parses out a timestamp corresponding to when the media was created (as we do with images).

Attachments (14)

35218.patch (6.5 KB) - added by stevegrunwell 5 years ago.
First pass at wp_get_media_creation_timestamp()
quicktime.mov (2.6 MB) - added by stevegrunwell 5 years ago.
Quicktime video used for testing
mp4.m4v (1.3 MB) - added by stevegrunwell 5 years ago.
MPEG-4 video used for testing
mkv.mkv (1.3 MB) - added by stevegrunwell 5 years ago.
MKV video used for testing
35218.diff (4.4 KB) - added by desrosj 3 years ago.
35218.2.diff (4.9 KB) - added by desrosj 3 years ago.
Add filter to wp_get_media_creation_timestamp()
35218.3.diff (783.7 KB) - added by blobfolio 3 years ago.
Add WebM handling, extra Matroska check, and include unit test files
35218.4.diff (784.2 KB) - added by blobfolio 3 years ago.
Merge desrosj's changes. :)
phpunit-data-uploads.zip (602.6 KB) - added by blobfolio 3 years ago.
These are the quickie files used in the unit tests, meant to be placed in tests/phpunit/data/uploads
35218.5.diff (6.4 KB) - added by blobfolio 3 years ago.
This is the same as 35218.4.diff, compiled without the --binary flag.
35218.6.diff (6.4 KB) - added by mikeschroder 3 years ago.
Only create meta if there is a creation date and only rely on small* test media files.
video_meta.diff (6.5 KB) - added by mikeschroder 3 years ago.
Place new data into video_meta array
video_meta_expanded_filter.diff (6.7 KB) - added by mikeschroder 3 years ago.
Place new data into video_meta array and add new video meta filter
35218.7.diff (6.5 KB) - added by mikeschroder 3 years ago.
Back to storing without image_meta array

Change History (39)

@stevegrunwell
5 years ago

First pass at wp_get_media_creation_timestamp()

@stevegrunwell
5 years ago

Quicktime video used for testing

@stevegrunwell
5 years ago

MPEG-4 video used for testing

@stevegrunwell
5 years ago

MKV video used for testing

#1 @stevegrunwell
5 years ago

Note: the patch file attached references three video files, which are also attached. These short, 15 second clips in three different formats should live in tests/phpunit/data/videos/

#2 @swissspidy
5 years ago

  • Keywords has-patch has-unit-tests added

#3 @chriscct7
4 years ago

  • Version trunk deleted

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


3 years ago

#5 @joemcgill
3 years ago

  • Milestone changed from Awaiting Review to 4.9
  • Owner set to joemcgill
  • Status changed from new to assigned

Thanks @stevegrunwell for the work on this and sorry that it's taken so long to review. Let's take a look for 4.9.

#6 @mikeschroder
3 years ago

I like this. Let's get some testing on it with additional files.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


3 years ago

#9 @mikeschroder
3 years ago

I dug into this a bit, and if I'm reading this correctly, it looks like we will need to do normalization on the values (or maybe grab from a different location/format for MOVs?).

The example MOV file, and one that I tested from a live photo, have creation dates that get stored as far in the future.

The one in the automated test, for example, is 3533823605 which looks to be 12/24/2081 @ 5:40pm (UTC).

Last edited 3 years ago by mikeschroder (previous) (diff)

#10 @desrosj
3 years ago

Looked into this and did some testing. Also noticed what @mikeschroder did. There is a creation_time_unix index for Quicktime/MP4 files. Is there any reason not to use that value for the created time instead of the creation_time one?

Incoming patch using that one instead, as well as updating the version in the inline documentation.

@desrosj
3 years ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


3 years ago

#12 @stevegrunwell
3 years ago

Totally on board with using creation_time_unix instead of creation_time, as long as it's consistent. The ID3 documentation has a nice warning about the "subatoms" node:

This is an undocumentably-complex recursive array, typically containing a huge amount of seemingly disorganized data. Avoid this like the plague.

A creation date in the far future doesn't do anybody any favors :)

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


3 years ago

#15 @joemcgill
3 years ago

  • Owner changed from joemcgill to mikeschroder
  • Status changed from assigned to reviewing

Reassigning to @mikeschroder for final decision, commit here.

@desrosj
3 years ago

Add filter to wp_get_media_creation_timestamp()

#16 @desrosj
3 years ago

Per discussion in today's weekly component meeting, 35218.2.diff adds a filter to the function result. This will allow devs to hook in and parse the creation time for any file type that we do not decide to parse in Core.

@blobfolio
3 years ago

Add WebM handling, extra Matroska check, and include unit test files

#17 @blobfolio
3 years ago

I updated the patch to include WebM. It also looks for Matroska creation times in a second place (an FFMPEG source file I tested with had it under [info] instead of [comments]). This patch also has binary data for some unit test files (all based off the existing small-video.mp4.

It would be nice to get some OGG love in here too, but getID3 doesn't seem to fully support Theora yet.

@blobfolio
3 years ago

Merge desrosj's changes. :)

#18 @mikeschroder
3 years ago

Thanks, @desrosj @blobfolio!

Looking at this today for commit.

@blobfolio
3 years ago

These are the quickie files used in the unit tests, meant to be placed in tests/phpunit/data/uploads

@blobfolio
3 years ago

This is the same as 35218.4.diff, compiled without the --binary flag.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


3 years ago

@mikeschroder
3 years ago

Only create meta if there is a creation date and only rely on small* test media files.

@mikeschroder
3 years ago

Place new data into video_meta array

@mikeschroder
3 years ago

Place new data into video_meta array and add new video meta filter

#20 @mikeschroder
3 years ago

Chatted with @joemcgill about this tonight.
Three new patches, can commit one of them in the morning (or feel free to do it, @joemcgill if it needs to happen first thing, and you're working first).

I'm okay with all of these approaches, but prefer the last.

  • 35218.6.diff is basically an update. Only create meta if there is a creation date and only rely on small* test media files.
  • video_meta.diff places the new data (creation timestamp) into a video_meta array in meta, much like image_meta on images.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


3 years ago

#22 @joemcgill
3 years ago

I like the idea of putting the filter in wp_read_video_metadata() as in video_meta_expanded_filter.diff, now with fresh eyes, I think putting creation time into a video_meta array seems unnecessary. In reality, everything returned by wp_read_video_metadata() is “video meta”, but we don’t want to change that storage at this point for back-compat reasons. Otherwise, this looks good for a commit today.

#23 @mikeschroder
3 years ago

Sure, @joemcgill, pick the one option I didn't leave a diff for!

I'm kidding, of course. Will work this up, thanks so much for the review!

Would appreciate any more eyes on the wp_read_video_metadata filter, since it had to be just enough different from the image one to make it not directly interchangable.

@mikeschroder
3 years ago

Back to storing without image_meta array

#24 @mikeschroder
3 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 41746:

Media: Store video creation date in meta.

When able to be parsed, store the created date for a video file from meta,
since this is useful separately from the dates on the file itself.

Introduces wp_get_media_creation_timestamp() to read the timestamp from
getID3 and a wp_read_video_metadata filter analogous to
wp_read_image_metadata.

Fixes #35218.
Props stevegrunwell, joemcgill, desrosj, blobfolio, mikeschroder.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.