WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#26825 closed enhancement (fixed)

Add ability to generate metadata for audio / video files on-demand

Reported by: wonderboymusic Owned by: nacin
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.6
Component: Media Keywords: has-patch
Focuses: Cc:

Description

If you uploaded audio/video files before 3.6, they do not have attachment metadata. There is currently no way to generate this on the fly. On the Edit Media screen, if we know there is no metadata for a media attachment, perhaps we show a button or whatnot that will generate the metadata (scan the item's ID3 tags) when pressed. This is especially important for video, so that we know the proper dimensions before rendering.

Attachments (5)

26825.diff (1.7 KB) - added by wonderboymusic 6 years ago.
26825-prepare.diff (927 bytes) - added by wonderboymusic 6 years ago.
26825.2.diff (1.1 KB) - added by wonderboymusic 6 years ago.
26825.3.diff (1.9 KB) - added by wonderboymusic 6 years ago.
26825.4.diff (798 bytes) - added by kovshenin 6 years ago.

Download all attachments as: .zip

Change History (23)

@wonderboymusic
6 years ago

#1 @wonderboymusic
6 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

26825.diff is first pass at making this happen. Would like someone to review it.

To test:

  • Upload an audio file.
  • Confirm that metadata was generated in the database.
  • Go to the Edit Media screen for the item.
  • View the metadata in the right sidebar.
  • Delete the metadata from the database.
  • Refresh the Edit Media screen.
  • Notice the "Read Metadata From File" button.
  • Click it.
  • Voila. Metadata is back.

#2 @bradyvercher
6 years ago

Nice. Instead of adding UI for this, is there any reason it shouldn't be generated automatically if the metadata key doesn't exist? It could be generated just by visiting the attachment edit screen and in wp_prepare_attachment_for_js().

The only thing that might be an issue is if the data can't be generated for some reason, doing it this way might cause wp_generate_attachment_metadata() to be run more than necessary, so maybe a flag would need to be set if automatically generating the metadata fails?

#3 @nacin
6 years ago

I agree with bradyvercher 100%. He kinds of hints at a race condition (of multiple processes trying to update metadata at once), just something else to think about.

This really something we should try as hard as possible to handle without involving the user.

#4 @wonderboymusic
6 years ago

Perhaps the postmeta key can be set to an empty value, so the only check will be if the key exists or not. For files that were previously uploaded before there was meta generated, there was no key at all

#5 follow-up: @wonderboymusic
6 years ago

26825-prepare.diff does the regeneration in wp_prepare_attachment_for_js() - but I think this is a bad idea. If your library has 300 videos in it, all uploaded before 3.6 - the first time you kick open the media modal, it will attempt the parse all 300 videos

#6 @helen
6 years ago

Can we regenerate on loading the edit media screen? Only generating if missing the key entirely seems smart. A race condition could happen, but seems pretty edge if specifically on that screen.

#7 in reply to: ↑ 5 ; follow-up: @bradyvercher
6 years ago

Replying to wonderboymusic:

Perhaps the postmeta key can be set to an empty value, so the only check will be if the key exists or not.

Using the meta key itself for the flag sounds like a good idea. I wasn't sure if the key could exist with an empty value yet.

Replying to wonderboymusic:

If your library has 300 videos in it, all uploaded before 3.6 - the first time you kick open the media modal, it will attempt the parse all 300 videos

When does core ever load 300 attachments at a time? It seems like 40 would be the most and it'd be a one-time thing.

Replying to helen:

Can we regenerate on loading the edit media screen?

I figured both places might be beneficial, but in the scenario where there are 300 videos without metadata, that'd require visiting each of those edit screens.

As far as race conditions, what's the worst that could happen aside from a file being processed twice? Once the metadata key exists, the file wouldn't be processed again. Is there a more extreme side effect?

#8 in reply to: ↑ 7 @wonderboymusic
6 years ago

Replying to bradyvercher:

When does core ever load 300 attachments at a time? It seems like 40 would be the most and it'd be a one-time thing.

The media modal makes an unbounded query for EVERY attachment when it is popped open the first time

#9 @wonderboymusic
6 years ago

26825.2.diff does this on the loading of the Edit Media page and implements locking via transient - a semaphore, as it were. So, no race condition.

This is the very least that needs to happen, might need to happen other places. Going to drop this in soon.

#10 @wonderboymusic
6 years ago

26825.3.diff is better

#11 @wonderboymusic
6 years ago

In 27127:

Introduce maybe_regenerate_attachment_metadata( $attachment ). On the Edit Media screen, call it for Audio and Video files.

The functions checks if the item is lacking metadata altogether. If a video or audio file was uploaded prior to 3.6, it does not have any metadata. This tries to fix it. Implements locking via a transient to protect against this running in parallel with another request.

This is the minimum viable product for #26825, but leaving the ticket open unless this function needs to be called in other places.

See #26825.

#12 @nacin
6 years ago

[27127] looks great.

maybe_regenerate_attachment_metadata() seems like something that could be suited as a default action. I don't think it works at the moment or for these particular contexts, but just wanted to mention as a potential off-the-wall idea.

#13 @bradyvercher
6 years ago

Replying to wonderboymusic:

The media modal makes an unbounded query for EVERY attachment when it is popped open the first time

If I'm reading the code right, it should only load 40 at a time and continue fetching with an infinite scroll technique. Here is the default posts per page arg. Inspecting the AJAX request seems to confirm the same.

In any case, I like the new method.

#14 @wonderboymusic
6 years ago

@bradyvercher: touché

#15 @nacin
6 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 27397:

Rename maybe_regenerate_attachment_metadata() to wp_maybe_generate_attachment_metadata(), to match wp_generate_attachment_metadata().

fixes #26825.

@kovshenin
6 years ago

#16 @kovshenin
6 years ago

  • Keywords dev-feedback removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

In media.php we're still referencing maybe_regenerate_attachment_metadata causing fatal errors, see 26825.4.diff.

This ticket was mentioned in IRC in #wordpress-dev by kovshenin. View the logs.


6 years ago

#18 @nacin
6 years ago

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

In 27441:

maybe_regenerate_attachment_metadata() is now wp_maybe_generate_attachment_metadata().

props kovshenin.
fixes #26825.

Note: See TracTickets for help on using tickets.